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

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

[Lina Iyer]

Hi all,

Changes since v1 [2] - 
- Addressed review comments from v1.
	- Fixes around dynamic allocation of genpd states
	- Used OF method for iterating phandles
	- Updated documentation, examples
	- Rename state variable (provider -> fwnode)
- The series is available at [3].

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: Save the fwnode in genpd_power_state
  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     |  38 +++
 Documentation/power/devices.txt                    |   9 +-
 arch/arm/mach-imx/gpc.c                            |  17 +-
 drivers/base/power/domain.c                        | 360 +++++++++++++++++----
 include/linux/pm_domain.h                          |  28 +-
 5 files changed, 380 insertions(+), 72 deletions(-)

-- 
2.7.4

[1]. https://www.spinics.net/lists/arm-kernel/msg526814.html
[2]. http://www.spinics.net/lists/arm-kernel/msg535106.html
[3]. https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-v2

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

[Lina Iyer]

Hi all,

Changes since v1 [2] - 
- Addressed review comments from v1.
	- Fixes around dynamic allocation of genpd states
	- Used OF method for iterating phandles
	- Updated documentation, examples
	- Rename state variable (provider -> fwnode)
- The series is available at [3].

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: Save the fwnode in genpd_power_state
  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     |  38 +++
 Documentation/power/devices.txt                    |   9 +-
 arch/arm/mach-imx/gpc.c                            |  17 +-
 drivers/base/power/domain.c                        | 360 +++++++++++++++++----
 include/linux/pm_domain.h                          |  28 +-
 5 files changed, 380 insertions(+), 72 deletions(-)

-- 
2.7.4

[1]. https://www.spinics.net/lists/arm-kernel/msg526814.html
[2]. http://www.spinics.net/lists/arm-kernel/msg535106.html
[3]. https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-v2

[PATCH v2 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 | 36 ++++++++++++++++++++++++------------
 include/linux/pm_domain.h   |  5 ++---
 3 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 0df062d..57a410b 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;
+	imx6q_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..4e87170 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1282,6 +1282,21 @@ out:
 }
 EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
 
+static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
+{
+	struct genpd_power_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	genpd->states = state;
+	genpd->state_count = 1;
+	genpd->free = state;
+
+	return 0;
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1293,6 +1308,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
 int pm_genpd_init(struct generic_pm_domain *genpd,
 		  struct dev_power_governor *gov, bool is_off)
 {
+	int ret;
+
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
@@ -1325,19 +1342,12 @@ 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;
+	if (genpd->state_count == 0) {
+		ret = genpd_set_default_power_state(genpd);
+		if (ret)
+			return ret;
+	}
 
 	mutex_lock(&gpd_list_lock);
 	list_add(&genpd->gpd_list_node, &gpd_list);
@@ -1374,6 +1384,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 		kfree(link);
 	}
 
+	kfree(genpd->free);
+
 	list_del(&genpd->gpd_list_node);
 	mutex_unlock(&genpd->lock);
 	cancel_work_sync(&genpd->power_off_work);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a09fe5c..de1d8f3 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,9 +68,10 @@ 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 */
+	void *free; /* Free the state that was allocated for default */
 
 };
 
-- 
2.7.4

[PATCH v2 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 | 36 ++++++++++++++++++++++++------------
 include/linux/pm_domain.h   |  5 ++---
 3 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 0df062d..57a410b 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;
+	imx6q_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..4e87170 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1282,6 +1282,21 @@ out:
 }
 EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
 
+static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
+{
+	struct genpd_power_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	genpd->states = state;
+	genpd->state_count = 1;
+	genpd->free = state;
+
+	return 0;
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1293,6 +1308,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
 int pm_genpd_init(struct generic_pm_domain *genpd,
 		  struct dev_power_governor *gov, bool is_off)
 {
+	int ret;
+
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
@@ -1325,19 +1342,12 @@ 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;
+	if (genpd->state_count == 0) {
+		ret = genpd_set_default_power_state(genpd);
+		if (ret)
+			return ret;
+	}
 
 	mutex_lock(&gpd_list_lock);
 	list_add(&genpd->gpd_list_node, &gpd_list);
@@ -1374,6 +1384,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 		kfree(link);
 	}
 
+	kfree(genpd->free);
+
 	list_del(&genpd->gpd_list_node);
 	mutex_unlock(&genpd->lock);
 	cancel_work_sync(&genpd->power_off_work);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a09fe5c..de1d8f3 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,9 +68,10 @@ 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 */
+	void *free; /* Free the state that was allocated for default */
 
 };
 
-- 
2.7.4

[PATCH v2 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>
Acked-by: Ulf Hansson <ulf.hansson@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 de1d8f3..f4492eb 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 v2 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>
Acked-by: Ulf Hansson <ulf.hansson@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 de1d8f3..f4492eb 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 v2 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. Add API to read the idle states from DT that can
be set in the genpd object.

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 | 95 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 ++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4e87170..4208b67 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1917,6 +1917,101 @@ 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 genpd_parse_state(struct genpd_power_state *genpd_state,
+				    struct device_node *state_node)
+{
+	int err;
+	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;
+
+	genpd_state->power_on_latency_ns = 1000 * exit_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 = 0;
+	int err, ret;
+	int count;
+	struct of_phandle_iterator it;
+
+	count = of_count_phandle_with_args(dn, "domain-idle-states", NULL);
+
+	st = kcalloc(count, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	/* Loop over the phandles until all the requested entry is found */
+	of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) {
+		np = of_node_get(it.node);
+		ret = genpd_parse_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);
+			goto fail;
+		}
+		of_node_put(np);
+	}
+
+	*n = count;
+	*states = st;
+
+	return 0;
+fail:
+	kfree(st);
+	return ret;
+}
+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 f4492eb..b489496 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -205,6 +205,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 */
@@ -234,6 +236,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 v2 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. Add API to read the idle states from DT that can
be set in the genpd object.

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 | 95 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 ++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4e87170..4208b67 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1917,6 +1917,101 @@ 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 genpd_parse_state(struct genpd_power_state *genpd_state,
+				    struct device_node *state_node)
+{
+	int err;
+	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;
+
+	genpd_state->power_on_latency_ns = 1000 * exit_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 = 0;
+	int err, ret;
+	int count;
+	struct of_phandle_iterator it;
+
+	count = of_count_phandle_with_args(dn, "domain-idle-states", NULL);
+
+	st = kcalloc(count, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	/* Loop over the phandles until all the requested entry is found */
+	of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) {
+		np = of_node_get(it.node);
+		ret = genpd_parse_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);
+			goto fail;
+		}
+		of_node_put(np);
+	}
+
+	*n = count;
+	*states = st;
+
+	return 0;
+fail:
+	kfree(st);
+	return ret;
+}
+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 f4492eb..b489496 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -205,6 +205,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 */
@@ -234,6 +236,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 v2 4/8] PM / Domains: Save the fwnode in genpd_power_state

[Lina Iyer]

Save the fwnode for the genpd state in the state node. PM Domain clients
may use the fwnode to read in the platform specific domain state
properties and associate them with the 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 4208b67..e0f31fe 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1957,6 +1957,7 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
 
 	genpd_state->power_on_latency_ns = 1000 * exit_latency;
 	genpd_state->power_off_latency_ns = 1000 * entry_latency;
+	genpd_state->fwnode = &state_node->fwnode;
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b489496..6a89881 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 *fwnode;
 };
 
 struct generic_pm_domain {
-- 
2.7.4

[PATCH v2 4/8] PM / Domains: Save the fwnode in genpd_power_state

[Lina Iyer]

Save the fwnode for the genpd state in the state node. PM Domain clients
may use the fwnode to read in the platform specific domain state
properties and associate them with the 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 4208b67..e0f31fe 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1957,6 +1957,7 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
 
 	genpd_state->power_on_latency_ns = 1000 * exit_latency;
 	genpd_state->power_off_latency_ns = 1000 * entry_latency;
+	genpd_state->fwnode = &state_node->fwnode;
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b489496..6a89881 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 *fwnode;
 };
 
 struct generic_pm_domain {
-- 
2.7.4

[PATCH v2 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     | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 025b5e7..7f8f27e 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@12340000 {
@@ -59,6 +63,38 @@ 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@12340000 {
+		compatible = "foo,power-controller";
+		reg = <0x12340000 0x1000>;
+		#power-domain-cells = <0>;
+		domain-idle-states = <&DOMAIN_RET>, <&DOMAIN_PWR_DN>;
+	};
+
+	child: power-controller@12341000 {
+		compatible = "foo,power-controller";
+		reg = <0x12341000 0x1000>;
+		power-domains = <&parent 0>;
+		#power-domain-cells = <0>;
+		domain-idle-states = <&DOMAIN_PWR_DN>;
+	};
+
+	DOMAIN_RET: state@0 {
+		compatible = "arm,idle-state";
+		reg = <0x0>;
+		entry-latency-us = <1000>;
+		exit-latency-us = <2000>;
+		min-residency-us = <10000>;
+	};
+
+	DOMAIN_PWR_DN: state@1 {
+		compatible = "arm,idle-state";
+		reg = <0x1>;
+		entry-latency-us = <5000>;
+		exit-latency-us = <8000>;
+		min-residency-us = <7000>;
+	};
+
 ==PM domain consumers==
 
 Required properties:
@@ -76,3 +112,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 v2 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     | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 025b5e7..7f8f27e 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,38 @@ 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 = <0>;
+		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 = <0>;
+		domain-idle-states = <&DOMAIN_PWR_DN>;
+	};
+
+	DOMAIN_RET: state at 0 {
+		compatible = "arm,idle-state";
+		reg = <0x0>;
+		entry-latency-us = <1000>;
+		exit-latency-us = <2000>;
+		min-residency-us = <10000>;
+	};
+
+	DOMAIN_PWR_DN: state at 1 {
+		compatible = "arm,idle-state";
+		reg = <0x1>;
+		entry-latency-us = <5000>;
+		exit-latency-us = <8000>;
+		min-residency-us = <7000>;
+	};
+
 ==PM domain consumers==
 
 Required properties:
@@ -76,3 +112,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 v2 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 e0f31fe..d0ae559 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_ops {
+	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_ops genpd_mtx_ops = {
+	.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_ops->lock(p)
+#define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
+#define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
+#define genpd_unlock(p)			p->lock_ops->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;
 }
@@ -1316,7 +1356,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_ops = &genpd_mtx_ops;
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
@@ -1364,16 +1405,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;
 	}
@@ -1387,7 +1428,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 	kfree(genpd->free);
 
 	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);
 
@@ -1910,9 +1951,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;
 }
@@ -2066,7 +2107,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;
 
@@ -2103,7 +2144,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 6a89881..811b968 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -42,13 +42,14 @@ struct genpd_power_state {
 	struct fwnode_handle *fwnode;
 };
 
+struct genpd_lock_ops;
+
 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 */
@@ -74,6 +75,8 @@ struct generic_pm_domain {
 	unsigned int state_count; /* number of states */
 	unsigned int state_idx; /* state that genpd will go to when off */
 	void *free; /* Free the state that was allocated for default */
+	const struct genpd_lock_ops *lock_ops;
+	struct mutex mlock;
 
 };
 
-- 
2.7.4

[PATCH v2 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 e0f31fe..d0ae559 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_ops {
+	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_ops genpd_mtx_ops = {
+	.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_ops->lock(p)
+#define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
+#define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
+#define genpd_unlock(p)			p->lock_ops->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;
 }
@@ -1316,7 +1356,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_ops = &genpd_mtx_ops;
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
@@ -1364,16 +1405,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;
 	}
@@ -1387,7 +1428,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 	kfree(genpd->free);
 
 	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);
 
@@ -1910,9 +1951,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;
 }
@@ -2066,7 +2107,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;
 
@@ -2103,7 +2144,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 6a89881..811b968 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -42,13 +42,14 @@ struct genpd_power_state {
 	struct fwnode_handle *fwnode;
 };
 
+struct genpd_lock_ops;
+
 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 */
@@ -74,6 +75,8 @@ struct generic_pm_domain {
 	unsigned int state_count; /* number of states */
 	unsigned int state_idx; /* state that genpd will go to when off */
 	void *free; /* Free the state that was allocated for default */
+	const struct genpd_lock_ops *lock_ops;
+	struct mutex mlock;
 
 };
 
-- 
2.7.4

[PATCH v2 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 | 111 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/pm_domain.h   |  10 +++-
 2 files changed, 110 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d0ae559..87a016a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = {
 	.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_ops genpd_spin_ops = {
+	.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_ops->lock(p)
 #define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
 #define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
 #define genpd_unlock(p)			p->lock_ops->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)
+{
+	bool ret;
+
+	ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
+
+	/* Warn once for each IRQ safe dev in no sleep domain */
+	if (ret)
+		dev_warn_once(dev, "PM domain %s will not be powered off\n",
+				genpd->name);
+
+	return ret;
+}
+
 /*
  * Get the generic PM domain for a particular struct device.
  * This validates the struct device pointer, the PM domain pointer,
@@ -343,7 +402,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 +570,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 +607,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 +653,8 @@ static int genpd_runtime_resume(struct device *dev)
 err_stop:
 	genpd_stop_dev(genpd, dev);
 err_poweroff:
-	if (!dev->power.irq_safe) {
+	if (!pm_runtime_is_irq_safe(dev) ||
+		(pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) {
 		genpd_lock(genpd);
 		genpd_poweroff(genpd, 0);
 		genpd_unlock(genpd);
@@ -1223,6 +1291,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;
@@ -1337,6 +1416,17 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
 	return 0;
 }
 
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+		spin_lock_init(&genpd->slock);
+		genpd->lock_ops = &genpd_spin_ops;
+	} else {
+		mutex_init(&genpd->mlock);
+		genpd->lock_ops = &genpd_mtx_ops;
+	}
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1356,8 +1446,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_ops = &genpd_mtx_ops;
+	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
@@ -2133,7 +2222,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 811b968..81ece61 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 */
@@ -76,7 +78,13 @@ struct generic_pm_domain {
 	unsigned int state_idx; /* state that genpd will go to when off */
 	void *free; /* Free the state that was allocated for default */
 	const struct genpd_lock_ops *lock_ops;
-	struct mutex mlock;
+	union {
+		struct mutex mlock;
+		struct {
+			spinlock_t slock;
+			unsigned long lock_flags;
+		};
+	};
 
 };
 
-- 
2.7.4

[PATCH v2 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 | 111 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/pm_domain.h   |  10 +++-
 2 files changed, 110 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d0ae559..87a016a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = {
 	.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_ops genpd_spin_ops = {
+	.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_ops->lock(p)
 #define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
 #define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
 #define genpd_unlock(p)			p->lock_ops->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)
+{
+	bool ret;
+
+	ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
+
+	/* Warn once for each IRQ safe dev in no sleep domain */
+	if (ret)
+		dev_warn_once(dev, "PM domain %s will not be powered off\n",
+				genpd->name);
+
+	return ret;
+}
+
 /*
  * Get the generic PM domain for a particular struct device.
  * This validates the struct device pointer, the PM domain pointer,
@@ -343,7 +402,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 +570,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 +607,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 +653,8 @@ static int genpd_runtime_resume(struct device *dev)
 err_stop:
 	genpd_stop_dev(genpd, dev);
 err_poweroff:
-	if (!dev->power.irq_safe) {
+	if (!pm_runtime_is_irq_safe(dev) ||
+		(pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) {
 		genpd_lock(genpd);
 		genpd_poweroff(genpd, 0);
 		genpd_unlock(genpd);
@@ -1223,6 +1291,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;
@@ -1337,6 +1416,17 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
 	return 0;
 }
 
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+		spin_lock_init(&genpd->slock);
+		genpd->lock_ops = &genpd_spin_ops;
+	} else {
+		mutex_init(&genpd->mlock);
+		genpd->lock_ops = &genpd_mtx_ops;
+	}
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1356,8 +1446,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_ops = &genpd_mtx_ops;
+	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
@@ -2133,7 +2222,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 811b968..81ece61 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 */
@@ -76,7 +78,13 @@ struct generic_pm_domain {
 	unsigned int state_idx; /* state that genpd will go to when off */
 	void *free; /* Free the state that was allocated for default */
 	const struct genpd_lock_ops *lock_ops;
-	struct mutex mlock;
+	union {
+		struct mutex mlock;
+		struct {
+			spinlock_t slock;
+			unsigned long lock_flags;
+		};
+	};
 
 };
 
-- 
2.7.4

[PATCH v2 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index 8ba6625..0401b53 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -607,7 +607,14 @@ 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 and PM domains may be defined as IRQ-safe, if they can be powered
+on/off even when the IRQs are disabled. An IRQ-safe device in a domain will
+disallow power management on the domain, unless the domain is also defined as
+IRQ-safe. The restriction this framework imposes on the parent domain of an
+IRQ-safe domain is that it must also be defined as IRQ-safe.
 
 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 v2 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index 8ba6625..0401b53 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -607,7 +607,14 @@ 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 and PM domains may be defined as IRQ-safe, if they can be powered
+on/off even when the IRQs are disabled. An IRQ-safe device in a domain will
+disallow power management on the domain, unless the domain is also defined as
+IRQ-safe. The restriction this framework imposes on the parent domain of an
+IRQ-safe domain is that it must also be defined as IRQ-safe.
 
 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

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

[Ulf Hansson]

On 8 October 2016 at 00:36, 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 | 36 ++++++++++++++++++++++++------------
>  include/linux/pm_domain.h   |  5 ++---
>  3 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index 0df062d..57a410b 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;
> +       imx6q_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..4e87170 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1282,6 +1282,21 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>
> +static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
> +{
> +       struct genpd_power_state *state;
> +
> +       state = kzalloc(sizeof(*state), GFP_KERNEL);
> +       if (!state)
> +               return -ENOMEM;
> +
> +       genpd->states = state;
> +       genpd->state_count = 1;
> +       genpd->free = state;
> +
> +       return 0;
> +}
> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1293,6 +1308,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>  int pm_genpd_init(struct generic_pm_domain *genpd,
>                   struct dev_power_governor *gov, bool is_off)
>  {
> +       int ret;
> +
>         if (IS_ERR_OR_NULL(genpd))
>                 return -EINVAL;
>
> @@ -1325,19 +1342,12 @@ 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;
> +       if (genpd->state_count == 0) {
> +               ret = genpd_set_default_power_state(genpd);
> +               if (ret)
> +                       return ret;
> +       }
>
>         mutex_lock(&gpd_list_lock);
>         list_add(&genpd->gpd_list_node, &gpd_list);
> @@ -1374,6 +1384,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>                 kfree(link);
>         }
>
> +       kfree(genpd->free);
> +

To be safe, let's move this after cancel_work_sync() - as to prevent
no accesses is made to ->states pointer after you have freed it.

>         list_del(&genpd->gpd_list_node);
>         mutex_unlock(&genpd->lock);
>         cancel_work_sync(&genpd->power_off_work);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a09fe5c..de1d8f3 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,9 +68,10 @@ 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 */
> +       void *free; /* Free the state that was allocated for default */
>
>  };
>
> --
> 2.7.4
>

After the minor change suggested above, you may add my ack.

Kind regards
Uffe

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

[Ulf Hansson]

On 8 October 2016 at 00:36, 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 | 36 ++++++++++++++++++++++++------------
>  include/linux/pm_domain.h   |  5 ++---
>  3 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index 0df062d..57a410b 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;
> +       imx6q_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..4e87170 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1282,6 +1282,21 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>
> +static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
> +{
> +       struct genpd_power_state *state;
> +
> +       state = kzalloc(sizeof(*state), GFP_KERNEL);
> +       if (!state)
> +               return -ENOMEM;
> +
> +       genpd->states = state;
> +       genpd->state_count = 1;
> +       genpd->free = state;
> +
> +       return 0;
> +}
> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1293,6 +1308,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>  int pm_genpd_init(struct generic_pm_domain *genpd,
>                   struct dev_power_governor *gov, bool is_off)
>  {
> +       int ret;
> +
>         if (IS_ERR_OR_NULL(genpd))
>                 return -EINVAL;
>
> @@ -1325,19 +1342,12 @@ 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;
> +       if (genpd->state_count == 0) {
> +               ret = genpd_set_default_power_state(genpd);
> +               if (ret)
> +                       return ret;
> +       }
>
>         mutex_lock(&gpd_list_lock);
>         list_add(&genpd->gpd_list_node, &gpd_list);
> @@ -1374,6 +1384,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>                 kfree(link);
>         }
>
> +       kfree(genpd->free);
> +

To be safe, let's move this after cancel_work_sync() - as to prevent
no accesses is made to ->states pointer after you have freed it.

>         list_del(&genpd->gpd_list_node);
>         mutex_unlock(&genpd->lock);
>         cancel_work_sync(&genpd->power_off_work);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a09fe5c..de1d8f3 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,9 +68,10 @@ 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 */
> +       void *free; /* Free the state that was allocated for default */
>
>  };
>
> --
> 2.7.4
>

After the minor change suggested above, you may add my ack.

Kind regards
Uffe

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

[Ulf Hansson]

On 8 October 2016 at 00:36, 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. Add API to read the idle states from DT that can
> be set in the genpd object.
>
> 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 | 95 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  8 ++++
>  2 files changed, 103 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 4e87170..4208b67 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1917,6 +1917,101 @@ 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 genpd_parse_state(struct genpd_power_state *genpd_state,
> +                                   struct device_node *state_node)
> +{
> +       int err;
> +       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;
> +
> +       genpd_state->power_on_latency_ns = 1000 * exit_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)

Instead of taking **states as a parameter, let's instead return it as
a pointer for the allocated struct. In case of failures, let's return
ERR_PTR().

> +{
> +       struct genpd_power_state *st;
> +       struct device_node *np;
> +       int i = 0;
> +       int err, ret;
> +       int count;
> +       struct of_phandle_iterator it;
> +
> +       count = of_count_phandle_with_args(dn, "domain-idle-states", NULL);

If count is zero or an error, we should return an error code (ERR_PTR()). Right?

> +
> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
> +       if (!st)
> +               return -ENOMEM;
> +
> +       /* Loop over the phandles until all the requested entry is found */
> +       of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) {
> +               np = of_node_get(it.node);

I don't think you need to increment the usage count for the device
node as that is already managed by of_for_each_phandle().

It's only in the error case below, when it's needed.

> +               ret = genpd_parse_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);
> +                       goto fail;

The goto seems unnecessary. Why not deal with all error handling here
and return the error code?

> +               }
> +               of_node_put(np);

According the comment above, you should be able to remove this.

> +       }
> +
> +       *n = count;
> +       *states = st;
> +
> +       return 0;
> +fail:
> +       kfree(st);
> +       return ret;
> +}
> +EXPORT_SYMBOL(of_genpd_parse_idle_states);

Please use EXPORT_SYMBOL_GPL() instead.

> +
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f4492eb..b489496 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -205,6 +205,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 */
> @@ -234,6 +236,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 v2 3/8] PM / Domains: Allow domain power states to be read from DT

[Ulf Hansson]

On 8 October 2016 at 00:36, 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. Add API to read the idle states from DT that can
> be set in the genpd object.
>
> 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 | 95 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  8 ++++
>  2 files changed, 103 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 4e87170..4208b67 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1917,6 +1917,101 @@ 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 genpd_parse_state(struct genpd_power_state *genpd_state,
> +                                   struct device_node *state_node)
> +{
> +       int err;
> +       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;
> +
> +       genpd_state->power_on_latency_ns = 1000 * exit_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)

Instead of taking **states as a parameter, let's instead return it as
a pointer for the allocated struct. In case of failures, let's return
ERR_PTR().

> +{
> +       struct genpd_power_state *st;
> +       struct device_node *np;
> +       int i = 0;
> +       int err, ret;
> +       int count;
> +       struct of_phandle_iterator it;
> +
> +       count = of_count_phandle_with_args(dn, "domain-idle-states", NULL);

If count is zero or an error, we should return an error code (ERR_PTR()). Right?

> +
> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
> +       if (!st)
> +               return -ENOMEM;
> +
> +       /* Loop over the phandles until all the requested entry is found */
> +       of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) {
> +               np = of_node_get(it.node);

I don't think you need to increment the usage count for the device
node as that is already managed by of_for_each_phandle().

It's only in the error case below, when it's needed.

> +               ret = genpd_parse_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);
> +                       goto fail;

The goto seems unnecessary. Why not deal with all error handling here
and return the error code?

> +               }
> +               of_node_put(np);

According the comment above, you should be able to remove this.

> +       }
> +
> +       *n = count;
> +       *states = st;
> +
> +       return 0;
> +fail:
> +       kfree(st);
> +       return ret;
> +}
> +EXPORT_SYMBOL(of_genpd_parse_idle_states);

Please use EXPORT_SYMBOL_GPL() instead.

> +
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f4492eb..b489496 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -205,6 +205,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 */
> @@ -234,6 +236,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

Re: [PATCH v2 4/8] PM / Domains: Save the fwnode in genpd_power_state

[Ulf Hansson]

On 8 October 2016 at 00:36, 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 platform specific domain state
> properties and associate them with the state.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

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

Kind regards
Uffe

> ---
>  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 4208b67..e0f31fe 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1957,6 +1957,7 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
>
>         genpd_state->power_on_latency_ns = 1000 * exit_latency;
>         genpd_state->power_off_latency_ns = 1000 * entry_latency;
> +       genpd_state->fwnode = &state_node->fwnode;
>
>         return 0;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b489496..6a89881 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 *fwnode;
>  };
>
>  struct generic_pm_domain {
> --
> 2.7.4
>

[PATCH v2 4/8] PM / Domains: Save the fwnode in genpd_power_state

[Ulf Hansson]

On 8 October 2016 at 00:36, 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 platform specific domain state
> properties and associate them with the state.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

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

Kind regards
Uffe

> ---
>  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 4208b67..e0f31fe 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1957,6 +1957,7 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
>
>         genpd_state->power_on_latency_ns = 1000 * exit_latency;
>         genpd_state->power_off_latency_ns = 1000 * entry_latency;
> +       genpd_state->fwnode = &state_node->fwnode;
>
>         return 0;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b489496..6a89881 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 *fwnode;
>  };
>
>  struct generic_pm_domain {
> --
> 2.7.4
>

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

[Ulf Hansson]

On 8 October 2016 at 00:37, 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>

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

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 111 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pm_domain.h   |  10 +++-
>  2 files changed, 110 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d0ae559..87a016a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = {
>         .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_ops genpd_spin_ops = {
> +       .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_ops->lock(p)
>  #define genpd_lock_nested(p, d)                p->lock_ops->lock_nested(p, d)
>  #define genpd_lock_interruptible(p)    p->lock_ops->lock_interruptible(p)
>  #define genpd_unlock(p)                        p->lock_ops->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)
> +{
> +       bool ret;
> +
> +       ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
> +
> +       /* Warn once for each IRQ safe dev in no sleep domain */
> +       if (ret)
> +               dev_warn_once(dev, "PM domain %s will not be powered off\n",
> +                               genpd->name);
> +
> +       return ret;
> +}
> +
>  /*
>   * Get the generic PM domain for a particular struct device.
>   * This validates the struct device pointer, the PM domain pointer,
> @@ -343,7 +402,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 +570,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 +607,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 +653,8 @@ static int genpd_runtime_resume(struct device *dev)
>  err_stop:
>         genpd_stop_dev(genpd, dev);
>  err_poweroff:
> -       if (!dev->power.irq_safe) {
> +       if (!pm_runtime_is_irq_safe(dev) ||
> +               (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) {
>                 genpd_lock(genpd);
>                 genpd_poweroff(genpd, 0);
>                 genpd_unlock(genpd);
> @@ -1223,6 +1291,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;
> @@ -1337,6 +1416,17 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
>         return 0;
>  }
>
> +static void genpd_lock_init(struct generic_pm_domain *genpd)
> +{
> +       if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> +               spin_lock_init(&genpd->slock);
> +               genpd->lock_ops = &genpd_spin_ops;
> +       } else {
> +               mutex_init(&genpd->mlock);
> +               genpd->lock_ops = &genpd_mtx_ops;
> +       }
> +}
> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1356,8 +1446,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_ops = &genpd_mtx_ops;
> +       genpd_lock_init(genpd);
>         genpd->gov = gov;
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         atomic_set(&genpd->sd_count, 0);
> @@ -2133,7 +2222,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 811b968..81ece61 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 */
> @@ -76,7 +78,13 @@ struct generic_pm_domain {
>         unsigned int state_idx; /* state that genpd will go to when off */
>         void *free; /* Free the state that was allocated for default */
>         const struct genpd_lock_ops *lock_ops;
> -       struct mutex mlock;
> +       union {
> +               struct mutex mlock;
> +               struct {
> +                       spinlock_t slock;
> +                       unsigned long lock_flags;
> +               };
> +       };
>
>  };
>
> --
> 2.7.4
>

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

[Ulf Hansson]

On 8 October 2016 at 00:37, 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>

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

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 111 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pm_domain.h   |  10 +++-
>  2 files changed, 110 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d0ae559..87a016a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -74,11 +74,70 @@ static const struct genpd_lock_ops genpd_mtx_ops = {
>         .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_ops genpd_spin_ops = {
> +       .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_ops->lock(p)
>  #define genpd_lock_nested(p, d)                p->lock_ops->lock_nested(p, d)
>  #define genpd_lock_interruptible(p)    p->lock_ops->lock_interruptible(p)
>  #define genpd_unlock(p)                        p->lock_ops->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)
> +{
> +       bool ret;
> +
> +       ret = pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
> +
> +       /* Warn once for each IRQ safe dev in no sleep domain */
> +       if (ret)
> +               dev_warn_once(dev, "PM domain %s will not be powered off\n",
> +                               genpd->name);
> +
> +       return ret;
> +}
> +
>  /*
>   * Get the generic PM domain for a particular struct device.
>   * This validates the struct device pointer, the PM domain pointer,
> @@ -343,7 +402,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 +570,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 +607,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 +653,8 @@ static int genpd_runtime_resume(struct device *dev)
>  err_stop:
>         genpd_stop_dev(genpd, dev);
>  err_poweroff:
> -       if (!dev->power.irq_safe) {
> +       if (!pm_runtime_is_irq_safe(dev) ||
> +               (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) {
>                 genpd_lock(genpd);
>                 genpd_poweroff(genpd, 0);
>                 genpd_unlock(genpd);
> @@ -1223,6 +1291,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;
> @@ -1337,6 +1416,17 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
>         return 0;
>  }
>
> +static void genpd_lock_init(struct generic_pm_domain *genpd)
> +{
> +       if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> +               spin_lock_init(&genpd->slock);
> +               genpd->lock_ops = &genpd_spin_ops;
> +       } else {
> +               mutex_init(&genpd->mlock);
> +               genpd->lock_ops = &genpd_mtx_ops;
> +       }
> +}
> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1356,8 +1446,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_ops = &genpd_mtx_ops;
> +       genpd_lock_init(genpd);
>         genpd->gov = gov;
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         atomic_set(&genpd->sd_count, 0);
> @@ -2133,7 +2222,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 811b968..81ece61 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 */
> @@ -76,7 +78,13 @@ struct generic_pm_domain {
>         unsigned int state_idx; /* state that genpd will go to when off */
>         void *free; /* Free the state that was allocated for default */
>         const struct genpd_lock_ops *lock_ops;
> -       struct mutex mlock;
> +       union {
> +               struct mutex mlock;
> +               struct {
> +                       spinlock_t slock;
> +                       unsigned long lock_flags;
> +               };
> +       };
>
>  };
>
> --
> 2.7.4
>

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

[Ulf Hansson]

On 8 October 2016 at 00:37, 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>

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

Kind regards
Uffe

> ---
>  Documentation/power/devices.txt | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..0401b53 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,14 @@ 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 and PM domains may be defined as IRQ-safe, if they can be powered
> +on/off even when the IRQs are disabled. An IRQ-safe device in a domain will
> +disallow power management on the domain, unless the domain is also defined as
> +IRQ-safe. The restriction this framework imposes on the parent domain of an
> +IRQ-safe domain is that it must also be defined as IRQ-safe.
>
>  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 v2 8/8] PM / doc: Update device documentation for devices in IRQ safe PM domains

[Ulf Hansson]

On 8 October 2016 at 00:37, 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>

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

Kind regards
Uffe

> ---
>  Documentation/power/devices.txt | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..0401b53 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,14 @@ 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 and PM domains may be defined as IRQ-safe, if they can be powered
> +on/off even when the IRQs are disabled. An IRQ-safe device in a domain will
> +disallow power management on the domain, unless the domain is also defined as
> +IRQ-safe. The restriction this framework imposes on the parent domain of an
> +IRQ-safe domain is that it must also be defined as IRQ-safe.
>
>  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
>

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

[Lina Iyer]

On Mon, Oct 10 2016 at 04:01 -0600, Ulf Hansson wrote:
>On 8 October 2016 at 00:36, 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. Add API to read the idle states from DT that can
>> be set in the genpd object.
>>
>> 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 | 95 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 ++++
>>  2 files changed, 103 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 4e87170..4208b67 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1917,6 +1917,101 @@ 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 genpd_parse_state(struct genpd_power_state *genpd_state,
>> +                                   struct device_node *state_node)
>> +{
>> +       int err;
>> +       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;
>> +
>> +       genpd_state->power_on_latency_ns = 1000 * exit_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)
>
>Instead of taking **states as a parameter, let's instead return it as
>a pointer for the allocated struct. In case of failures, let's return
>ERR_PTR().
>
Hmm.. I thought about it. There are 2 return values from this function.
If we return a pointer to the allocated memory, we still have to return
the size of it as an argument. I wasn't happy splitting the return
values in 2 different places.

>> +{
>> +       struct genpd_power_state *st;
>> +       struct device_node *np;
>> +       int i = 0;
>> +       int err, ret;
>> +       int count;
>> +       struct of_phandle_iterator it;
>> +
>> +       count = of_count_phandle_with_args(dn, "domain-idle-states", NULL);
>
>If count is zero or an error, we should return an error code (ERR_PTR()). Right?
>
OK
>> +
>> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
>> +       if (!st)
>> +               return -ENOMEM;
>> +
>> +       /* Loop over the phandles until all the requested entry is found */
>> +       of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) {
>> +               np = of_node_get(it.node);
>
>I don't think you need to increment the usage count for the device
>node as that is already managed by of_for_each_phandle().
>
>It's only in the error case below, when it's needed.
>
Hmm.. Didn't realize that.. will fix.

>> +               ret = genpd_parse_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);
>> +                       goto fail;
>
>The goto seems unnecessary. Why not deal with all error handling here
>and return the error code?
>
>> +               }
>> +               of_node_put(np);
>
>According the comment above, you should be able to remove this.
>
>> +       }
>> +
>> +       *n = count;
>> +       *states = st;
>> +
>> +       return 0;
>> +fail:
>> +       kfree(st);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
>
>Please use EXPORT_SYMBOL_GPL() instead.
>
Hmm.. OK

Thanks,
Lina
>> +
>>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>>
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index f4492eb..b489496 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -205,6 +205,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 */
>> @@ -234,6 +236,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 v2 3/8] PM / Domains: Allow domain power states to be read from DT

[Lina Iyer]

On Mon, Oct 10 2016 at 04:01 -0600, Ulf Hansson wrote:
>On 8 October 2016 at 00:36, 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. Add API to read the idle states from DT that can
>> be set in the genpd object.
>>
>> 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 | 95 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 ++++
>>  2 files changed, 103 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 4e87170..4208b67 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1917,6 +1917,101 @@ 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 genpd_parse_state(struct genpd_power_state *genpd_state,
>> +                                   struct device_node *state_node)
>> +{
>> +       int err;
>> +       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;
>> +
>> +       genpd_state->power_on_latency_ns = 1000 * exit_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)
>
>Instead of taking **states as a parameter, let's instead return it as
>a pointer for the allocated struct. In case of failures, let's return
>ERR_PTR().
>
Hmm.. I thought about it. There are 2 return values from this function.
If we return a pointer to the allocated memory, we still have to return
the size of it as an argument. I wasn't happy splitting the return
values in 2 different places.

>> +{
>> +       struct genpd_power_state *st;
>> +       struct device_node *np;
>> +       int i = 0;
>> +       int err, ret;
>> +       int count;
>> +       struct of_phandle_iterator it;
>> +
>> +       count = of_count_phandle_with_args(dn, "domain-idle-states", NULL);
>
>If count is zero or an error, we should return an error code (ERR_PTR()). Right?
>
OK
>> +
>> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
>> +       if (!st)
>> +               return -ENOMEM;
>> +
>> +       /* Loop over the phandles until all the requested entry is found */
>> +       of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) {
>> +               np = of_node_get(it.node);
>
>I don't think you need to increment the usage count for the device
>node as that is already managed by of_for_each_phandle().
>
>It's only in the error case below, when it's needed.
>
Hmm.. Didn't realize that.. will fix.

>> +               ret = genpd_parse_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);
>> +                       goto fail;
>
>The goto seems unnecessary. Why not deal with all error handling here
>and return the error code?
>
>> +               }
>> +               of_node_put(np);
>
>According the comment above, you should be able to remove this.
>
>> +       }
>> +
>> +       *n = count;
>> +       *states = st;
>> +
>> +       return 0;
>> +fail:
>> +       kfree(st);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
>
>Please use EXPORT_SYMBOL_GPL() instead.
>
Hmm.. OK

Thanks,
Lina
>> +
>>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>>
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index f4492eb..b489496 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -205,6 +205,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 */
>> @@ -234,6 +236,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

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

[Lina Iyer]

On Mon, Oct 10 2016 at 02:40 -0600, Ulf Hansson wrote:
>On 8 October 2016 at 00:36, 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 | 36 ++++++++++++++++++++++++------------
>>  include/linux/pm_domain.h   |  5 ++---
>>  3 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
>> index 0df062d..57a410b 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;
>> +       imx6q_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..4e87170 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1282,6 +1282,21 @@ out:
>>  }
>>  EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>>
>> +static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
>> +{
>> +       struct genpd_power_state *state;
>> +
>> +       state = kzalloc(sizeof(*state), GFP_KERNEL);
>> +       if (!state)
>> +               return -ENOMEM;
>> +
>> +       genpd->states = state;
>> +       genpd->state_count = 1;
>> +       genpd->free = state;
>> +
>> +       return 0;
>> +}
>> +
>>  /**
>>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>>   * @genpd: PM domain object to initialize.
>> @@ -1293,6 +1308,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>>  int pm_genpd_init(struct generic_pm_domain *genpd,
>>                   struct dev_power_governor *gov, bool is_off)
>>  {
>> +       int ret;
>> +
>>         if (IS_ERR_OR_NULL(genpd))
>>                 return -EINVAL;
>>
>> @@ -1325,19 +1342,12 @@ 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;
>> +       if (genpd->state_count == 0) {
>> +               ret = genpd_set_default_power_state(genpd);
>> +               if (ret)
>> +                       return ret;
>> +       }
>>
>>         mutex_lock(&gpd_list_lock);
>>         list_add(&genpd->gpd_list_node, &gpd_list);
>> @@ -1374,6 +1384,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>>                 kfree(link);
>>         }
>>
>> +       kfree(genpd->free);
>> +
>
>To be safe, let's move this after cancel_work_sync() - as to prevent
>no accesses is made to ->states pointer after you have freed it.
>
OK

Thanks,
Lina

>>         list_del(&genpd->gpd_list_node);
>>         mutex_unlock(&genpd->lock);
>>         cancel_work_sync(&genpd->power_off_work);
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index a09fe5c..de1d8f3 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,9 +68,10 @@ 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 */
>> +       void *free; /* Free the state that was allocated for default */
>>
>>  };
>>
>> --
>> 2.7.4
>>
>
>After the minor change suggested above, you may add my ack.
>
>Kind regards
>Uffe

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

[Lina Iyer]

On Mon, Oct 10 2016 at 02:40 -0600, Ulf Hansson wrote:
>On 8 October 2016 at 00:36, 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 | 36 ++++++++++++++++++++++++------------
>>  include/linux/pm_domain.h   |  5 ++---
>>  3 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
>> index 0df062d..57a410b 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;
>> +       imx6q_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..4e87170 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1282,6 +1282,21 @@ out:
>>  }
>>  EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>>
>> +static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
>> +{
>> +       struct genpd_power_state *state;
>> +
>> +       state = kzalloc(sizeof(*state), GFP_KERNEL);
>> +       if (!state)
>> +               return -ENOMEM;
>> +
>> +       genpd->states = state;
>> +       genpd->state_count = 1;
>> +       genpd->free = state;
>> +
>> +       return 0;
>> +}
>> +
>>  /**
>>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>>   * @genpd: PM domain object to initialize.
>> @@ -1293,6 +1308,8 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
>>  int pm_genpd_init(struct generic_pm_domain *genpd,
>>                   struct dev_power_governor *gov, bool is_off)
>>  {
>> +       int ret;
>> +
>>         if (IS_ERR_OR_NULL(genpd))
>>                 return -EINVAL;
>>
>> @@ -1325,19 +1342,12 @@ 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;
>> +       if (genpd->state_count == 0) {
>> +               ret = genpd_set_default_power_state(genpd);
>> +               if (ret)
>> +                       return ret;
>> +       }
>>
>>         mutex_lock(&gpd_list_lock);
>>         list_add(&genpd->gpd_list_node, &gpd_list);
>> @@ -1374,6 +1384,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>>                 kfree(link);
>>         }
>>
>> +       kfree(genpd->free);
>> +
>
>To be safe, let's move this after cancel_work_sync() - as to prevent
>no accesses is made to ->states pointer after you have freed it.
>
OK

Thanks,
Lina

>>         list_del(&genpd->gpd_list_node);
>>         mutex_unlock(&genpd->lock);
>>         cancel_work_sync(&genpd->power_off_work);
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index a09fe5c..de1d8f3 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,9 +68,10 @@ 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 */
>> +       void *free; /* Free the state that was allocated for default */
>>
>>  };
>>
>> --
>> 2.7.4
>>
>
>After the minor change suggested above, you may add my ack.
>
>Kind regards
>Uffe

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

[Sudeep Holla]

On 07/10/16 23:36, Lina Iyer 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     | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
> index 025b5e7..7f8f27e 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].
> +

Please do add the following details to the binding. IMO, this binding is
not complete in terms of specification as there are few open questions:

1. What not define a standard compatible instead of "arm,idle-state" ?
    I agree it can be used, but as part of this *generic* binding, IMO
    it's better to have something generic and can be used by devices.
    Otherwise, this binding becomes CPU specific, that too ARM CPU
    specific.

2. Now taking CPU as a special device, how does this co-exist with the
    cpu-idle-states ? Better to have some description may be in the ARM
    CPU idle binding document(not here of-course)

3. I still haven't seen any explanation for not considering complete
    hierarchical power domain representation which was raised in earlier
    versions. I had provided example for the proposal. I just saw them
    already in use in the upstream kernel by Renasas. e.g.:
    arch/arm/boot/dts/r8a73a4.dtsi

    How does that fit with your proposal, though you have not made one
    yet for CPUs in this binding ? In the above file, CPUs have either
    own power domain inside the L2 one which is cluster level power
    domain.

One must be able to get answers to these above questions with this
binding. Until then it's *incomplete* though it may be correct.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 07/10/16 23:36, Lina Iyer 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     | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
> index 025b5e7..7f8f27e 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].
> +

Please do add the following details to the binding. IMO, this binding is
not complete in terms of specification as there are few open questions:

1. What not define a standard compatible instead of "arm,idle-state" ?
    I agree it can be used, but as part of this *generic* binding, IMO
    it's better to have something generic and can be used by devices.
    Otherwise, this binding becomes CPU specific, that too ARM CPU
    specific.

2. Now taking CPU as a special device, how does this co-exist with the
    cpu-idle-states ? Better to have some description may be in the ARM
    CPU idle binding document(not here of-course)

3. I still haven't seen any explanation for not considering complete
    hierarchical power domain representation which was raised in earlier
    versions. I had provided example for the proposal. I just saw them
    already in use in the upstream kernel by Renasas. e.g.:
    arch/arm/boot/dts/r8a73a4.dtsi

    How does that fit with your proposal, though you have not made one
    yet for CPUs in this binding ? In the above file, CPUs have either
    own power domain inside the L2 one which is cluster level power
    domain.

One must be able to get answers to these above questions with this
binding. Until then it's *incomplete* though it may be correct.

-- 
Regards,
Sudeep

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

[Lina Iyer]

On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>
>
>On 07/10/16 23:36, Lina Iyer 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     | 38 ++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
>>index 025b5e7..7f8f27e 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].
>>+
>
>Please do add the following details to the binding. IMO, this binding is
>not complete in terms of specification as there are few open questions:
>
>1. What not define a standard compatible instead of "arm,idle-state" ?
>   I agree it can be used, but as part of this *generic* binding, IMO
>   it's better to have something generic and can be used by devices.
>   Otherwise, this binding becomes CPU specific, that too ARM CPU
>   specific.
>
We had gone down this path of having a separate DT bindings for domains
that is not arm,idle-state. See RFC patches. But the binding did closely
match this and it so was suggested that we use arm,idle-state which is
already defined.

>2. Now taking CPU as a special device, how does this co-exist with the
>   cpu-idle-states ? Better to have some description may be in the ARM
>   CPU idle binding document(not here of-course)
>
The is a binding for a generic PM domain. This has no bearing on the CPU
or its idle states. Its just that the data is compatible with
arm,idle-state.

>3. I still haven't seen any explanation for not considering complete
>   hierarchical power domain representation which was raised in earlier
>   versions. I had provided example for the proposal. I just saw them
>   already in use in the upstream kernel by Renasas. e.g.:
>   arch/arm/boot/dts/r8a73a4.dtsi
>
Hierarchical power domains have been available for few years in DT. The
OF features of domains have always supported it. Platforms are free to
define domains in hierarchy they seem fit for their SoCs. This is a
feature that is available today and is not being modified in these
patches. It will be creating confusion if I talk about hierarchical
domains which are obvious and irrelevant to this series.

>   How does that fit with your proposal, though you have not made one
>   yet for CPUs in this binding ? In the above file, CPUs have either
>   own power domain inside the L2 one which is cluster level power
>   domain.
>
Again, this series is not about the CPUs. This is about adding features
to genpd that may be used in other contexts including cpuidle in the
future.

>One must be able to get answers to these above questions with this
>binding. Until then it's *incomplete* though it may be correct.
>
I have always tried to answer all your questions. If anything remains
unclarified pls. bring it up.

Thanks,
Lina

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

[Lina Iyer]

On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>
>
>On 07/10/16 23:36, Lina Iyer 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     | 38 ++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
>>index 025b5e7..7f8f27e 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].
>>+
>
>Please do add the following details to the binding. IMO, this binding is
>not complete in terms of specification as there are few open questions:
>
>1. What not define a standard compatible instead of "arm,idle-state" ?
>   I agree it can be used, but as part of this *generic* binding, IMO
>   it's better to have something generic and can be used by devices.
>   Otherwise, this binding becomes CPU specific, that too ARM CPU
>   specific.
>
We had gone down this path of having a separate DT bindings for domains
that is not arm,idle-state. See RFC patches. But the binding did closely
match this and it so was suggested that we use arm,idle-state which is
already defined.

>2. Now taking CPU as a special device, how does this co-exist with the
>   cpu-idle-states ? Better to have some description may be in the ARM
>   CPU idle binding document(not here of-course)
>
The is a binding for a generic PM domain. This has no bearing on the CPU
or its idle states. Its just that the data is compatible with
arm,idle-state.

>3. I still haven't seen any explanation for not considering complete
>   hierarchical power domain representation which was raised in earlier
>   versions. I had provided example for the proposal. I just saw them
>   already in use in the upstream kernel by Renasas. e.g.:
>   arch/arm/boot/dts/r8a73a4.dtsi
>
Hierarchical power domains have been available for few years in DT. The
OF features of domains have always supported it. Platforms are free to
define domains in hierarchy they seem fit for their SoCs. This is a
feature that is available today and is not being modified in these
patches. It will be creating confusion if I talk about hierarchical
domains which are obvious and irrelevant to this series.

>   How does that fit with your proposal, though you have not made one
>   yet for CPUs in this binding ? In the above file, CPUs have either
>   own power domain inside the L2 one which is cluster level power
>   domain.
>
Again, this series is not about the CPUs. This is about adding features
to genpd that may be used in other contexts including cpuidle in the
future.

>One must be able to get answers to these above questions with this
>binding. Until then it's *incomplete* though it may be correct.
>
I have always tried to answer all your questions. If anything remains
unclarified pls. bring it up.

Thanks,
Lina

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

[Sudeep Holla]

On 10/10/16 17:43, Lina Iyer wrote:
> On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>>
>>
>> On 07/10/16 23:36, Lina Iyer wrote:
>>> Update DT bindings to describe idle states of PM domains.
>>>
>>> This patch is based on the original patch by Marc Titinger.
>>>
>>> Cc: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>>> Signed-off-by: Marc Titinger <mtitinger+renesas-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> ---
>>> .../devicetree/bindings/power/power_domain.txt     | 38
>>> ++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
>>> b/Documentation/devicetree/bindings/power/power_domain.txt
>>> index 025b5e7..7f8f27e 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].
>>> +
>>
>> Please do add the following details to the binding. IMO, this binding is
>> not complete in terms of specification as there are few open questions:
>>
>> 1. What not define a standard compatible instead of "arm,idle-state" ?
>>   I agree it can be used, but as part of this *generic* binding, IMO
>>   it's better to have something generic and can be used by devices.
>>   Otherwise, this binding becomes CPU specific, that too ARM CPU
>>   specific.
>>
> We had gone down this path of having a separate DT bindings for domains
> that is not arm,idle-state. See RFC patches. But the binding did closely
> match this and it so was suggested that we use arm,idle-state which is
> already defined.
>

Either we say this binding is ARM CPU specific or generic, I can't
understand this mix 'n' match really. You have removed all the CPUIdle
stuff from this series which is good and makes it simpler, but linking
it to only "arm,idle-state" make be feel it's not generic. OK I will
have a look at the RFC as why generic compatible was rejected.

>> 2. Now taking CPU as a special device, how does this co-exist with the
>>   cpu-idle-states ? Better to have some description may be in the ARM
>>   CPU idle binding document(not here of-course)
>>
> The is a binding for a generic PM domain. This has no bearing on the CPU
> or its idle states. Its just that the data is compatible with
> arm,idle-state.
>

I understand that but it's not that simple which I assume you *do*
agree. Hence may need bit of an explanation in the binding(not here
of-course as I mentioned earlier, but in the CPU Idle bindings).
Please consider DT bindings as any other specification. All I am
asking is more description in the binding.

>> 3. I still haven't seen any explanation for not considering complete
>>   hierarchical power domain representation which was raised in earlier
>>   versions. I had provided example for the proposal. I just saw them
>>   already in use in the upstream kernel by Renasas. e.g.:
>>   arch/arm/boot/dts/r8a73a4.dtsi
>>
> Hierarchical power domains have been available for few years in DT. The
> OF features of domains have always supported it. Platforms are free to
> define domains in hierarchy they seem fit for their SoCs. This is a
> feature that is available today and is not being modified in these
> patches. It will be creating confusion if I talk about hierarchical
> domains which are obvious and irrelevant to this series.
>

Agreed and sorry if I created any confusion. But this binding doesn't
clearly state how to build up the hierarchy if the leaf node is not a
power-domain node and I am just trying have those clarifications in the
binding. It would be good if those details are *explicitly* mentioned in
the binding, not this particularly, but in CPU Idle one when you
introduce the user of that.

>>   How does that fit with your proposal, though you have not made one
>>   yet for CPUs in this binding ? In the above file, CPUs have either
>>   own power domain inside the L2 one which is cluster level power
>>   domain.
>>
> Again, this series is not about the CPUs. This is about adding features
> to genpd that may be used in other contexts including cpuidle in the
> future.
>

Yes I understand and hence I was confused as why I don't see an
*generic* compatible but just *arm,idle-states* in the example.

>> One must be able to get answers to these above questions with this
>> binding. Until then it's *incomplete* though it may be correct.
>>
> I have always tried to answer all your questions. If anything remains
> unclarified pls. bring it up.
>

It's not about questions, and definitely you have answered all my
questions, no argument there at all. Now we need to make those useful
discussions part of this binding so that it's *self explanatory* and
one need not refer back these discussions when writing DT for some
different SoC which differs from this. Again that could be part of
your CPUIdle series, I just raised it here as it got mixed sense from
this series. It was hard to be not to associate CPUIdle for reasons
mentioned above.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sudeep Holla]

On 10/10/16 17:43, Lina Iyer wrote:
> On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>>
>>
>> On 07/10/16 23:36, Lina Iyer 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     | 38
>>> ++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
>>> b/Documentation/devicetree/bindings/power/power_domain.txt
>>> index 025b5e7..7f8f27e 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].
>>> +
>>
>> Please do add the following details to the binding. IMO, this binding is
>> not complete in terms of specification as there are few open questions:
>>
>> 1. What not define a standard compatible instead of "arm,idle-state" ?
>>   I agree it can be used, but as part of this *generic* binding, IMO
>>   it's better to have something generic and can be used by devices.
>>   Otherwise, this binding becomes CPU specific, that too ARM CPU
>>   specific.
>>
> We had gone down this path of having a separate DT bindings for domains
> that is not arm,idle-state. See RFC patches. But the binding did closely
> match this and it so was suggested that we use arm,idle-state which is
> already defined.
>

Either we say this binding is ARM CPU specific or generic, I can't
understand this mix 'n' match really. You have removed all the CPUIdle
stuff from this series which is good and makes it simpler, but linking
it to only "arm,idle-state" make be feel it's not generic. OK I will
have a look at the RFC as why generic compatible was rejected.

>> 2. Now taking CPU as a special device, how does this co-exist with the
>>   cpu-idle-states ? Better to have some description may be in the ARM
>>   CPU idle binding document(not here of-course)
>>
> The is a binding for a generic PM domain. This has no bearing on the CPU
> or its idle states. Its just that the data is compatible with
> arm,idle-state.
>

I understand that but it's not that simple which I assume you *do*
agree. Hence may need bit of an explanation in the binding(not here
of-course as I mentioned earlier, but in the CPU Idle bindings).
Please consider DT bindings as any other specification. All I am
asking is more description in the binding.

>> 3. I still haven't seen any explanation for not considering complete
>>   hierarchical power domain representation which was raised in earlier
>>   versions. I had provided example for the proposal. I just saw them
>>   already in use in the upstream kernel by Renasas. e.g.:
>>   arch/arm/boot/dts/r8a73a4.dtsi
>>
> Hierarchical power domains have been available for few years in DT. The
> OF features of domains have always supported it. Platforms are free to
> define domains in hierarchy they seem fit for their SoCs. This is a
> feature that is available today and is not being modified in these
> patches. It will be creating confusion if I talk about hierarchical
> domains which are obvious and irrelevant to this series.
>

Agreed and sorry if I created any confusion. But this binding doesn't
clearly state how to build up the hierarchy if the leaf node is not a
power-domain node and I am just trying have those clarifications in the
binding. It would be good if those details are *explicitly* mentioned in
the binding, not this particularly, but in CPU Idle one when you
introduce the user of that.

>>   How does that fit with your proposal, though you have not made one
>>   yet for CPUs in this binding ? In the above file, CPUs have either
>>   own power domain inside the L2 one which is cluster level power
>>   domain.
>>
> Again, this series is not about the CPUs. This is about adding features
> to genpd that may be used in other contexts including cpuidle in the
> future.
>

Yes I understand and hence I was confused as why I don't see an
*generic* compatible but just *arm,idle-states* in the example.

>> One must be able to get answers to these above questions with this
>> binding. Until then it's *incomplete* though it may be correct.
>>
> I have always tried to answer all your questions. If anything remains
> unclarified pls. bring it up.
>

It's not about questions, and definitely you have answered all my
questions, no argument there at all. Now we need to make those useful
discussions part of this binding so that it's *self explanatory* and
one need not refer back these discussions when writing DT for some
different SoC which differs from this. Again that could be part of
your CPUIdle series, I just raised it here as it got mixed sense from
this series. It was hard to be not to associate CPUIdle for reasons
mentioned above.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 10/10/16 17:43, Lina Iyer wrote:
> On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>>
>>
>> On 07/10/16 23:36, Lina Iyer 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     | 38
>>> ++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
>>> b/Documentation/devicetree/bindings/power/power_domain.txt
>>> index 025b5e7..7f8f27e 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].
>>> +
>>
>> Please do add the following details to the binding. IMO, this binding is
>> not complete in terms of specification as there are few open questions:
>>
>> 1. What not define a standard compatible instead of "arm,idle-state" ?
>>   I agree it can be used, but as part of this *generic* binding, IMO
>>   it's better to have something generic and can be used by devices.
>>   Otherwise, this binding becomes CPU specific, that too ARM CPU
>>   specific.
>>
> We had gone down this path of having a separate DT bindings for domains
> that is not arm,idle-state. See RFC patches. But the binding did closely
> match this and it so was suggested that we use arm,idle-state which is
> already defined.
>

Are you referring to [1] here ? Sorry, I did some search quickly and
could find this, wanted to check if I am looking at the right one ?

-- 
Regards,
Sudeep

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/353261.html

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

[Sudeep Holla]

On 10/10/16 17:43, Lina Iyer wrote:
> On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>>
>>
>> On 07/10/16 23:36, Lina Iyer 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     | 38
>>> ++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
>>> b/Documentation/devicetree/bindings/power/power_domain.txt
>>> index 025b5e7..7f8f27e 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].
>>> +
>>
>> Please do add the following details to the binding. IMO, this binding is
>> not complete in terms of specification as there are few open questions:
>>
>> 1. What not define a standard compatible instead of "arm,idle-state" ?
>>   I agree it can be used, but as part of this *generic* binding, IMO
>>   it's better to have something generic and can be used by devices.
>>   Otherwise, this binding becomes CPU specific, that too ARM CPU
>>   specific.
>>
> We had gone down this path of having a separate DT bindings for domains
> that is not arm,idle-state. See RFC patches. But the binding did closely
> match this and it so was suggested that we use arm,idle-state which is
> already defined.
>

Are you referring to [1] here ? Sorry, I did some search quickly and
could find this, wanted to check if I am looking at the right one ?

-- 
Regards,
Sudeep

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/353261.html

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

[Ulf Hansson]

[...]

>>> +int of_genpd_parse_idle_states(struct device_node *dn,
>>> +                       struct genpd_power_state **states, int *n)
>>
>>
>> Instead of taking **states as a parameter, let's instead return it as
>> a pointer for the allocated struct. In case of failures, let's return
>> ERR_PTR().
>>
> Hmm.. I thought about it. There are 2 return values from this function.
> If we return a pointer to the allocated memory, we still have to return
> the size of it as an argument. I wasn't happy splitting the return
> values in 2 different places.

Ahh, right. So let's keep it as is!

[...]

Kind regards
Uffe

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

[Ulf Hansson]

[...]

>>> +int of_genpd_parse_idle_states(struct device_node *dn,
>>> +                       struct genpd_power_state **states, int *n)
>>
>>
>> Instead of taking **states as a parameter, let's instead return it as
>> a pointer for the allocated struct. In case of failures, let's return
>> ERR_PTR().
>>
> Hmm.. I thought about it. There are 2 return values from this function.
> If we return a pointer to the allocated memory, we still have to return
> the size of it as an argument. I wasn't happy splitting the return
> values in 2 different places.

Ahh, right. So let's keep it as is!

[...]

Kind regards
Uffe

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

[Lina Iyer]

On Mon, Oct 10 2016 at 11:13 -0600, Sudeep Holla wrote:
>
>
>On 10/10/16 17:43, Lina Iyer wrote:
>>On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>>>
>>>
>>>On 07/10/16 23:36, Lina Iyer 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     | 38
>>>>++++++++++++++++++++++
>>>>1 file changed, 38 insertions(+)
>>>>
>>>>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
>>>>b/Documentation/devicetree/bindings/power/power_domain.txt
>>>>index 025b5e7..7f8f27e 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].
>>>>+
>>>
>>>Please do add the following details to the binding. IMO, this binding is
>>>not complete in terms of specification as there are few open questions:
>>>
>>>1. What not define a standard compatible instead of "arm,idle-state" ?
>>>  I agree it can be used, but as part of this *generic* binding, IMO
>>>  it's better to have something generic and can be used by devices.
>>>  Otherwise, this binding becomes CPU specific, that too ARM CPU
>>>  specific.
>>>
>>We had gone down this path of having a separate DT bindings for domains
>>that is not arm,idle-state. See RFC patches. But the binding did closely
>>match this and it so was suggested that we use arm,idle-state which is
>>already defined.
>>
>
>Either we say this binding is ARM CPU specific or generic, I can't
>understand this mix 'n' match really. You have removed all the CPUIdle
>stuff from this series which is good and makes it simpler, but linking
>it to only "arm,idle-state" make be feel it's not generic. OK I will
>have a look at the RFC as why generic compatible was rejected.
>
I will look for the discussion around it as well. A initial look through
didn't get me the thread I was looking for.

>>>2. Now taking CPU as a special device, how does this co-exist with the
>>>  cpu-idle-states ? Better to have some description may be in the ARM
>>>  CPU idle binding document(not here of-course)
>>>
>>The is a binding for a generic PM domain. This has no bearing on the CPU
>>or its idle states. Its just that the data is compatible with
>>arm,idle-state.
>>
>
>I understand that but it's not that simple which I assume you *do*
>agree. Hence may need bit of an explanation in the binding(not here
>of-course as I mentioned earlier, but in the CPU Idle bindings).
>Please consider DT bindings as any other specification. All I am
>asking is more description in the binding.
>
Any ideas of what description you would like to see? It seemed fairly
explanatory in the idle-states.txt, so I didn't go into depth here.

>>>3. I still haven't seen any explanation for not considering complete
>>>  hierarchical power domain representation which was raised in earlier
>>>  versions. I had provided example for the proposal. I just saw them
>>>  already in use in the upstream kernel by Renasas. e.g.:
>>>  arch/arm/boot/dts/r8a73a4.dtsi
>>>
>>Hierarchical power domains have been available for few years in DT. The
>>OF features of domains have always supported it. Platforms are free to
>>define domains in hierarchy they seem fit for their SoCs. This is a
>>feature that is available today and is not being modified in these
>>patches. It will be creating confusion if I talk about hierarchical
>>domains which are obvious and irrelevant to this series.
>>
>
>Agreed and sorry if I created any confusion. But this binding doesn't
>clearly state how to build up the hierarchy if the leaf node is not a
>power-domain node and I am just trying have those clarifications in the
>binding. It would be good if those details are *explicitly* mentioned in
>the binding, not this particularly, but in CPU Idle one when you
>introduce the user of that.
>
As we have today, devices have their own way of figuring out their idle
states, they are not represented in DT (CPU being an exception). 

>>>  How does that fit with your proposal, though you have not made one
>>>  yet for CPUs in this binding ? In the above file, CPUs have either
>>>  own power domain inside the L2 one which is cluster level power
>>>  domain.
>>>
>>Again, this series is not about the CPUs. This is about adding features
>>to genpd that may be used in other contexts including cpuidle in the
>>future.
>>
>
>Yes I understand and hence I was confused as why I don't see an
>*generic* compatible but just *arm,idle-states* in the example.
>
>>>One must be able to get answers to these above questions with this
>>>binding. Until then it's *incomplete* though it may be correct.
>>>
>>I have always tried to answer all your questions. If anything remains
>>unclarified pls. bring it up.
>>
>
>It's not about questions, and definitely you have answered all my
>questions, no argument there at all. Now we need to make those useful
>discussions part of this binding so that it's *self explanatory* and
>one need not refer back these discussions when writing DT for some
>different SoC which differs from this. Again that could be part of
>your CPUIdle series, I just raised it here as it got mixed sense from
>this series. It was hard to be not to associate CPUIdle for reasons
>mentioned above.
>

Thanks,
Lina

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

[Lina Iyer]

On Mon, Oct 10 2016 at 11:13 -0600, Sudeep Holla wrote:
>
>
>On 10/10/16 17:43, Lina Iyer wrote:
>>On Mon, Oct 10 2016 at 09:45 -0600, Sudeep Holla wrote:
>>>
>>>
>>>On 07/10/16 23:36, Lina Iyer 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     | 38
>>>>++++++++++++++++++++++
>>>>1 file changed, 38 insertions(+)
>>>>
>>>>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
>>>>b/Documentation/devicetree/bindings/power/power_domain.txt
>>>>index 025b5e7..7f8f27e 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].
>>>>+
>>>
>>>Please do add the following details to the binding. IMO, this binding is
>>>not complete in terms of specification as there are few open questions:
>>>
>>>1. What not define a standard compatible instead of "arm,idle-state" ?
>>>  I agree it can be used, but as part of this *generic* binding, IMO
>>>  it's better to have something generic and can be used by devices.
>>>  Otherwise, this binding becomes CPU specific, that too ARM CPU
>>>  specific.
>>>
>>We had gone down this path of having a separate DT bindings for domains
>>that is not arm,idle-state. See RFC patches. But the binding did closely
>>match this and it so was suggested that we use arm,idle-state which is
>>already defined.
>>
>
>Either we say this binding is ARM CPU specific or generic, I can't
>understand this mix 'n' match really. You have removed all the CPUIdle
>stuff from this series which is good and makes it simpler, but linking
>it to only "arm,idle-state" make be feel it's not generic. OK I will
>have a look at the RFC as why generic compatible was rejected.
>
I will look for the discussion around it as well. A initial look through
didn't get me the thread I was looking for.

>>>2. Now taking CPU as a special device, how does this co-exist with the
>>>  cpu-idle-states ? Better to have some description may be in the ARM
>>>  CPU idle binding document(not here of-course)
>>>
>>The is a binding for a generic PM domain. This has no bearing on the CPU
>>or its idle states. Its just that the data is compatible with
>>arm,idle-state.
>>
>
>I understand that but it's not that simple which I assume you *do*
>agree. Hence may need bit of an explanation in the binding(not here
>of-course as I mentioned earlier, but in the CPU Idle bindings).
>Please consider DT bindings as any other specification. All I am
>asking is more description in the binding.
>
Any ideas of what description you would like to see? It seemed fairly
explanatory in the idle-states.txt, so I didn't go into depth here.

>>>3. I still haven't seen any explanation for not considering complete
>>>  hierarchical power domain representation which was raised in earlier
>>>  versions. I had provided example for the proposal. I just saw them
>>>  already in use in the upstream kernel by Renasas. e.g.:
>>>  arch/arm/boot/dts/r8a73a4.dtsi
>>>
>>Hierarchical power domains have been available for few years in DT. The
>>OF features of domains have always supported it. Platforms are free to
>>define domains in hierarchy they seem fit for their SoCs. This is a
>>feature that is available today and is not being modified in these
>>patches. It will be creating confusion if I talk about hierarchical
>>domains which are obvious and irrelevant to this series.
>>
>
>Agreed and sorry if I created any confusion. But this binding doesn't
>clearly state how to build up the hierarchy if the leaf node is not a
>power-domain node and I am just trying have those clarifications in the
>binding. It would be good if those details are *explicitly* mentioned in
>the binding, not this particularly, but in CPU Idle one when you
>introduce the user of that.
>
As we have today, devices have their own way of figuring out their idle
states, they are not represented in DT (CPU being an exception). 

>>>  How does that fit with your proposal, though you have not made one
>>>  yet for CPUs in this binding ? In the above file, CPUs have either
>>>  own power domain inside the L2 one which is cluster level power
>>>  domain.
>>>
>>Again, this series is not about the CPUs. This is about adding features
>>to genpd that may be used in other contexts including cpuidle in the
>>future.
>>
>
>Yes I understand and hence I was confused as why I don't see an
>*generic* compatible but just *arm,idle-states* in the example.
>
>>>One must be able to get answers to these above questions with this
>>>binding. Until then it's *incomplete* though it may be correct.
>>>
>>I have always tried to answer all your questions. If anything remains
>>unclarified pls. bring it up.
>>
>
>It's not about questions, and definitely you have answered all my
>questions, no argument there at all. Now we need to make those useful
>discussions part of this binding so that it's *self explanatory* and
>one need not refer back these discussions when writing DT for some
>different SoC which differs from this. Again that could be part of
>your CPUIdle series, I just raised it here as it got mixed sense from
>this series. It was hard to be not to associate CPUIdle for reasons
>mentioned above.
>

Thanks,
Lina

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

[Sudeep Holla]

On 10/10/16 23:13, Lina Iyer wrote:
> On Mon, Oct 10 2016 at 11:13 -0600, Sudeep Holla wrote:

[...]

>> Either we say this binding is ARM CPU specific or generic, I can't
>> understand this mix 'n' match really. You have removed all the
>> CPUIdle stuff from this series which is good and makes it simpler,
>> but linking it to only "arm,idle-state" make be feel it's not
>> generic. OK I will have a look at the RFC as why generic compatible
>> was rejected.
>>
> I will look for the discussion around it as well. A initial look
> through didn't get me the thread I was looking for.
>

Sure

[...]

>>
>> I understand that but it's not that simple which I assume you *do*
>> agree. Hence may need bit of an explanation in the binding(not here
>> of-course as I mentioned earlier, but in the CPU Idle bindings).
>> Please consider DT bindings as any other specification. All I am
>> asking is more description in the binding.
>>
> Any ideas of what description you would like to see? It seemed fairly
> explanatory in the idle-states.txt, so I didn't go into depth here.
>

Various use cases we discussed and what takes precedence,... etc
E.g.: if the Renasas example I pointed out had cpu-idle-states and
power-domain but no domain-idle-states which is perfectly valid without
this bindings.

Basically all the important this we have discussed so far. Even the
OSC/PCC is worth mentioning so that we are explicitly clear that this
binding has no affiliation to those PSCI methods. In short it should be
able to answer any question one might get if is completely new to this
binding but is aware of old one.

[...]

>>
>> Agreed and sorry if I created any confusion. But this binding doesn't
>> clearly state how to build up the hierarchy if the leaf node is not a
>> power-domain node and I am just trying have those clarifications in the
>> binding. It would be good if those details are *explicitly* mentioned in
>> the binding, not this particularly, but in CPU Idle one when you
>> introduce the user of that.
>>
> As we have today, devices have their own way of figuring out their idle
> states, they are not represented in DT (CPU being an exception).

I understand that, and I assume this binding will provide a way to
represent that for devices too if required. No ? Otherwise I see no
point in just saying it's generic.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 10/10/16 23:13, Lina Iyer wrote:
> On Mon, Oct 10 2016 at 11:13 -0600, Sudeep Holla wrote:

[...]

>> Either we say this binding is ARM CPU specific or generic, I can't
>> understand this mix 'n' match really. You have removed all the
>> CPUIdle stuff from this series which is good and makes it simpler,
>> but linking it to only "arm,idle-state" make be feel it's not
>> generic. OK I will have a look at the RFC as why generic compatible
>> was rejected.
>>
> I will look for the discussion around it as well. A initial look
> through didn't get me the thread I was looking for.
>

Sure

[...]

>>
>> I understand that but it's not that simple which I assume you *do*
>> agree. Hence may need bit of an explanation in the binding(not here
>> of-course as I mentioned earlier, but in the CPU Idle bindings).
>> Please consider DT bindings as any other specification. All I am
>> asking is more description in the binding.
>>
> Any ideas of what description you would like to see? It seemed fairly
> explanatory in the idle-states.txt, so I didn't go into depth here.
>

Various use cases we discussed and what takes precedence,... etc
E.g.: if the Renasas example I pointed out had cpu-idle-states and
power-domain but no domain-idle-states which is perfectly valid without
this bindings.

Basically all the important this we have discussed so far. Even the
OSC/PCC is worth mentioning so that we are explicitly clear that this
binding has no affiliation to those PSCI methods. In short it should be
able to answer any question one might get if is completely new to this
binding but is aware of old one.

[...]

>>
>> Agreed and sorry if I created any confusion. But this binding doesn't
>> clearly state how to build up the hierarchy if the leaf node is not a
>> power-domain node and I am just trying have those clarifications in the
>> binding. It would be good if those details are *explicitly* mentioned in
>> the binding, not this particularly, but in CPU Idle one when you
>> introduce the user of that.
>>
> As we have today, devices have their own way of figuring out their idle
> states, they are not represented in DT (CPU being an exception).

I understand that, and I assume this binding will provide a way to
represent that for devices too if required. No ? Otherwise I see no
point in just saying it's generic.

-- 
Regards,
Sudeep