PM/Domains: Cluster idle support for ARM SoCs

From: Axel Haslam <[email protected]>

From: Axel Haslam <[email protected]>

Add the core changes to be able to declare multiple states. When trying to set a power domain to off, genpd will be able to choose from an array of states declared by the platform. The power on and off latencies are now tied to a state.

States should be declared in ascending order from shallowest to deepest,

deepest meaning the state which takes longer to enter and exit.

the power_off and power_on function can use the 'state_idx' field of the generic_pm_domain structure, to distinguish between the different states and act accordingly.

Example:

static int pd1_power_on(struct generic_pm_domain *domain)

{
        /* domain->state_idx = state the domain is coming from */
}

static int pd1_power_off(struct generic_pm_domain *domain)

{
        /* domain->state_idx = desired powered off state */
}

const struct genpd_power_state pd_states[] = {
        {
                .name = "RET",
                .power_on_latency_ns = ON_LATENCY_FAST,
                .power_off_latency_ns = OFF_LATENCY_FAST,
        },
        {
                .name = "DEEP_RET",
                .power_on_latency_ns = ON_LATENCY_MED,
                .power_off_latency_ns = OFF_LATENCY_MED,
        },
        {
                .name = "OFF",
                .power_on_latency_ns = ON_LATENCY_SLOW,
                .power_off_latency_ns = OFF_LATENCY_SLOW,
        }
};

struct generic_pm_domain pd1 = {
        .name = "PD1",
        .power_on = pd1_power_on,
        .power_off = pd1_power_off,
        [...]
};

int xxx_init_pm_domain(){

        pd1->state = copy_of(pd_states);
        pd1->state_count = ARRAY_SIZE(pd_states);
        pm_genpd_init(pd1, true);

}

Signed-off-by Axel Haslam <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: Modified genpd state initialization and remove use of save_state_latency_ns in genpd timing data]

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c          | 136 +++++++++++++++++++++++++++++++++--
 drivers/base/power/domain_governor.c |  13 +++-
 include/linux/pm_domain.h            |  10 +++
 3 files changed, 151 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +0 -0

index e03b1ad..3242854 100644

drivers/base/power/domain.c +11 -4

+                                      const struct genpd_power_state *st,
+                                      unsigned int st_count);
+
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 

@@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
 
 static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
+       unsigned int state_idx = genpd->state_idx;
        ktime_t time_start;
        s64 elapsed_ns;
        int ret;

@@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
                return ret;
 
        elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-       if (elapsed_ns <= genpd->power_on_latency_ns)
+       if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
                return ret;
 
-       genpd->power_on_latency_ns = elapsed_ns;
+       genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
        genpd->max_off_time_changed = true;
        pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
                 genpd->name, "on", elapsed_ns);

@@ -131,6 +138,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 
 static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 {
+       unsigned int state_idx = genpd->state_idx;
        ktime_t time_start;
        s64 elapsed_ns;
        int ret;

@@ -147,10 +155,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
                return ret;
 
        elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-       if (elapsed_ns <= genpd->power_off_latency_ns)
+       if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
                return ret;
 
-       genpd->power_off_latency_ns = elapsed_ns;
+       genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
        genpd->max_off_time_changed = true;
        pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
                 genpd->name, "off", elapsed_ns);

@@ -582,6 +590,8 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd,
            || atomic_read(&genpd->sd_count) > 0)
                return;
 
+       /* Choose the deepest state when suspending */
+       genpd->state_idx = genpd->state_count - 1;
        genpd_power_off(genpd, timed);
 
        genpd->status = GPD_STATE_POWER_OFF;

drivers/base/power/domain.c +38 -0

+                                  const struct genpd_power_state *st,
+                                  unsigned int st_count)
+{
+       int ret = 0;
+       unsigned int i;
+
+       if (IS_ERR_OR_NULL(genpd)) {
+               ret = -EINVAL;
+               goto err;
+       }
+
+       if (!st || (st_count < 1)) {
+               ret = -EINVAL;
+               goto err;
+       }
+
+       /* Allocate the local memory to keep the states for this genpd */
+       genpd->states = kcalloc(st_count, sizeof(*st), GFP_KERNEL);
+       if (!genpd->states) {
+               ret = -ENOMEM;
+               goto err;
+       }
+
+       for (i = 0; i < st_count; i++) {
+               genpd->states[i].power_on_latency_ns =
+                       st[i].power_on_latency_ns;
+               genpd->states[i].power_off_latency_ns =
+                       st[i].power_off_latency_ns;
+       }
+
+       /*
+        * Copy the latency values To keep compatibility with
+        * platforms that are not converted to use the multiple states.
+        * This will be removed once all platforms are converted to use
+        * multiple states. note that non converted platforms will use the
+        * default single off state.
+        */
+       if (genpd->power_on_latency_ns != 0)

drivers/base/power/domain.c +6 -0

+       pm_genpd_alloc_states_names(genpd, st, st_count);
+
+err:
+       return ret;
+}
+
 /**
  * __pm_genpd_add_device - Add a device to an I/O PM domain.
  * @genpd: PM domain to add the device to.

@@ -1459,6 +1524,15 @@ static int pm_genpd_default_restore_state(struct device *dev)
 void pm_genpd_init(struct generic_pm_domain *genpd,

drivers/base/power/domain.c +1 -0

+
        if (IS_ERR_OR_NULL(genpd))
                return;
 

drivers/base/power/domain.c +41 -1

+
        mutex_lock(&gpd_list_lock);
        list_add(&genpd->gpd_list_node, &gpd_list);
        mutex_unlock(&gpd_list_lock);

@@ -1822,6 +1909,33 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
 #include <linux/kobject.h>
 static struct dentry *pm_genpd_debugfs_dir;
 
+static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+                                      const struct genpd_power_state *st,
+                                      unsigned int st_count)
+{
+       unsigned int i;
+
+       if (IS_ERR_OR_NULL(genpd))
+               return -EINVAL;
+
+       if (genpd->state_count != st_count) {
+               pr_err("Invalid allocated state count\n");
+               return -EINVAL;
+       }
+
+       for (i = 0; i < st_count; i++) {
+               genpd->states[i].name = kstrndup(st[i].name,
+                               GENPD_MAX_NAME_SIZE, GFP_KERNEL);
+               if (!genpd->states[i].name) {
+                       pr_err("%s Failed to allocate state %d name.\n",
+                               genpd->name, i);
+                       return -ENOMEM;
+               }
+       }
+
+       return 0;
+}
+
 /*
  * TODO: This function is a slightly modified version of rtpm_status_show
  * from sysfs.c, so generalize it.

@@ -1855,6 +1969,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
                [GPD_STATE_ACTIVE] = "on",
                [GPD_STATE_POWER_OFF] = "off"
        };
+       unsigned int state_idx = genpd->state_idx;
        struct pm_domain_data *pm_data;
        const char *kobj_path;
        struct gpd_link *link;

@@ -1866,7 +1981,11 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
        if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup)))
                goto exit;
-       seq_printf(s, "%-30s  %-15s ", genpd->name, status_lookup[genpd->status]);
+
+       seq_printf(s, "%-30s  %-15s  ", genpd->name,
+                  (genpd->status == GPD_STATE_POWER_OFF) ?
+                  genpd->states[state_idx].name :
+                  status_lookup[genpd->status]);
 
        /*
         * Modifications on the list require holding locks on both

@@ -1954,4 +2073,11 @@ static void __exit pm_genpd_debug_exit(void)
        debugfs_remove_recursive(pm_genpd_debugfs_dir);
 }
 __exitcall(pm_genpd_debug_exit);
+#else
+static inline int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+                                       const struct genpd_power_state *st,
+                                       unsigned int st_count)
+{
+       return 0;
+}
 #endif /* CONFIG_PM_ADVANCED_DEBUG */

drivers/base/power/domain_governor.c +0 -0

index e60dd12..b4984f5 100644

include/linux/pm_domain.h +8 -1

+       s64 power_off_latency_ns;
+       s64 power_on_latency_ns;
+};
+
 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 */

@@ -66,6 +72,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;
+       unsigned int state_count; /* number of states */
+       unsigned int state_idx; /* state that genpd will go to when off */
+
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
--

2.1.4

Quoted 8 lines · Lina Iyer

From: Axel Haslam <[email protected]>

From: Axel Haslam <[email protected]>

Add the core changes to be able to declare multiple states. When trying to set a power domain to off, genpd will be able to choose from an array of states declared by the platform. The power on and off latencies are now tied to a state.

I would like to get an answer to *why* we need this.

For example, we should mention that some HWs supports these multiple states - at least.

Quoted 2 lines

States should be declared in ascending order from shallowest to deepest,

I guess *should* isn't good enough. Perhaps *shall* is better?

Quoted 5 lines

deepest meaning the state which takes longer to enter and exit.

the power_off and power_on function can use the 'state_idx' field of the generic_pm_domain structure, to distinguish between the different states and act accordingly.

This needs some more explanation.

First, please use the wording of "callbacks", like ->power_on() to better describe what function you are talking about. Second, what is "state_idx"? What's does it really tell the SOC PM domain driver here?

Quoted 38 lines

Example:

static int pd1_power_on(struct generic_pm_domain *domain)

{
        /* domain->state_idx = state the domain is coming from */
}

static int pd1_power_off(struct generic_pm_domain *domain)

{
        /* domain->state_idx = desired powered off state */
}

const struct genpd_power_state pd_states[] = {
        {
                .name = "RET",
                .power_on_latency_ns = ON_LATENCY_FAST,
                .power_off_latency_ns = OFF_LATENCY_FAST,
        },
        {
                .name = "DEEP_RET",
                .power_on_latency_ns = ON_LATENCY_MED,
                .power_off_latency_ns = OFF_LATENCY_MED,
        },
        {
                .name = "OFF",
                .power_on_latency_ns = ON_LATENCY_SLOW,
                .power_off_latency_ns = OFF_LATENCY_SLOW,
        }
};

struct generic_pm_domain pd1 = {
        .name = "PD1",
        .power_on = pd1_power_on,
        .power_off = pd1_power_off,
        [...]
};

Quoted 7 lines

int xxx_init_pm_domain(){

        pd1->state = copy_of(pd_states);
        pd1->state_count = ARRAY_SIZE(pd_states);
        pm_genpd_init(pd1, true);

}

Well, even if the above describes this quite good, I think it better belongs in the Documentation rather than in the change log.

Can we try to the improve the text in the change-log instead?

Quoted 18 lines

Signed-off-by Axel Haslam <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: Modified genpd state initialization and remove use of save_state_latency_ns in genpd timing data]

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c          | 136 +++++++++++++++++++++++++++++++++--
 drivers/base/power/domain_governor.c |  13 +++-
 include/linux/pm_domain.h            |  10 +++
 3 files changed, 151 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +0 -0

index e03b1ad..3242854 100644

drivers/base/power/domain.c line 39

Quoted 6 lines

drivers/base/power/domain.c +3 -0

@@ -34,6 +34,12 @@
        __ret;                                                  \
 })
 
+#define GENPD_MAX_NAME_SIZE 20
+
+static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,

Please avoid forward declarations.

Also, we have just got rid of all *name* based genpd API/functions. I don't want us to start adding another.

Or perhaps it's just the name of the function that I don't like!? :-)

Quoted 58 lines

drivers/base/power/domain.c +11 -4

+                                      const struct genpd_power_state *st,
+                                      unsigned int st_count);
+
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 

@@ -102,6 +108,7 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
 
 static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
+       unsigned int state_idx = genpd->state_idx;
        ktime_t time_start;
        s64 elapsed_ns;
        int ret;

@@ -118,10 +125,10 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
                return ret;
 
        elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-       if (elapsed_ns <= genpd->power_on_latency_ns)
+       if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
                return ret;
 
-       genpd->power_on_latency_ns = elapsed_ns;
+       genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
        genpd->max_off_time_changed = true;
        pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
                 genpd->name, "on", elapsed_ns);

@@ -131,6 +138,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 
 static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 {
+       unsigned int state_idx = genpd->state_idx;
        ktime_t time_start;
        s64 elapsed_ns;
        int ret;

@@ -147,10 +155,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
                return ret;
 
        elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-       if (elapsed_ns <= genpd->power_off_latency_ns)
+       if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
                return ret;
 
-       genpd->power_off_latency_ns = elapsed_ns;
+       genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
        genpd->max_off_time_changed = true;
        pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
                 genpd->name, "off", elapsed_ns);

@@ -582,6 +590,8 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd,
            || atomic_read(&genpd->sd_count) > 0)
                return;
 
+       /* Choose the deepest state when suspending */
+       genpd->state_idx = genpd->state_count - 1;
        genpd_power_off(genpd, timed);
 
        genpd->status = GPD_STATE_POWER_OFF;

drivers/base/power/domain.c static void genpd_free_dev_data(struct device *dev, · line 1218

Quoted 4 lines

drivers/base/power/domain.c +1 -0

@@ -1205,6 +1215,61 @@ static void genpd_free_dev_data(struct device *dev,
        dev_pm_put_subsys_data(dev);
 }
 
+static int genpd_alloc_states_data(struct generic_pm_domain *genpd,

In this patch I would rather just add *one* new "alloc" function and name it like below.

static int genpd_alloc_default_state(struct generic_pm_domain *genpd)

I assume the name I suggest for it, indicates what it needs to do and *not* needs to do.

Quoted 39 lines

drivers/base/power/domain.c +38 -0

+                                  const struct genpd_power_state *st,
+                                  unsigned int st_count)
+{
+       int ret = 0;
+       unsigned int i;
+
+       if (IS_ERR_OR_NULL(genpd)) {
+               ret = -EINVAL;
+               goto err;
+       }
+
+       if (!st || (st_count < 1)) {
+               ret = -EINVAL;
+               goto err;
+       }
+
+       /* Allocate the local memory to keep the states for this genpd */
+       genpd->states = kcalloc(st_count, sizeof(*st), GFP_KERNEL);
+       if (!genpd->states) {
+               ret = -ENOMEM;
+               goto err;
+       }
+
+       for (i = 0; i < st_count; i++) {
+               genpd->states[i].power_on_latency_ns =
+                       st[i].power_on_latency_ns;
+               genpd->states[i].power_off_latency_ns =
+                       st[i].power_off_latency_ns;
+       }
+
+       /*
+        * Copy the latency values To keep compatibility with
+        * platforms that are not converted to use the multiple states.
+        * This will be removed once all platforms are converted to use
+        * multiple states. note that non converted platforms will use the
+        * default single off state.
+        */
+       if (genpd->power_on_latency_ns != 0)

drivers/base/power/domain.c static void genpd_free_dev_data(struct device *dev, · line 1266

Quoted 10 lines

drivers/base/power/domain.c +10 -0

+               genpd->states[0].power_on_latency_ns =
+                               genpd->power_on_latency_ns;
+
+       if (genpd->power_off_latency_ns != 0)
+               genpd->states[0].power_off_latency_ns =
+                               genpd->power_off_latency_ns;
+
+       genpd->state_count = st_count;
+
+       /* to save memory, Name allocation will happen if debug is enabled */

Urgh. :-)

I instead suggest we skip the entire "name" attribute" and just use the state_idx to provide the same information.

For sure at this point, this information won't be interpreted by "anybody". Reading an integer value instead of string, shouldn't be that difficult to understand.

Future wise, if we see that it could be beneficial to add the "name" attribute, we can do that then.

Removing the name attribute will also simplify this patch, quite much. Can you please do that.

Quoted 11 lines

drivers/base/power/domain.c +6 -0

+       pm_genpd_alloc_states_names(genpd, st, st_count);
+
+err:
+       return ret;
+}
+
 /**
  * __pm_genpd_add_device - Add a device to an I/O PM domain.
  * @genpd: PM domain to add the device to.

@@ -1459,6 +1524,15 @@ static int pm_genpd_default_restore_state(struct device *dev)
 void pm_genpd_init(struct generic_pm_domain *genpd,

drivers/base/power/domain.c static int pm_genpd_default_restore_state(struct device *dev) · line 1534

Quoted 10 lines

drivers/base/power/domain.c +8 -0

                   struct dev_power_governor *gov, bool is_off)
 {
+       int ret;
+       static const struct genpd_power_state genpd_default_off[] = {
+               {
+                       .name = "OFF",
+                       .power_off_latency_ns = 0,
+                       .power_on_latency_ns = 0,
+               },
+       };

I suggest you remove this and instead handle that within genpd_alloc_default_state().

Quoted 5 lines

drivers/base/power/domain.c +1 -0

+
        if (IS_ERR_OR_NULL(genpd))
                return;
 

drivers/base/power/domain.c void pm_genpd_init(struct generic_pm_domain *genpd, · line 1585

Quoted 9 lines

drivers/base/power/domain.c +6 -0

@@ -1503,6 +1577,19 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
                genpd->dev_ops.start = pm_clk_resume;
        }
 
+       /*
+        * Allocate the default state if genpd states
+        * are not already defined.
+        */
+       if (!genpd->state_count) {
+               ret = genpd_alloc_states_data(genpd, genpd_default_off, 1);

Call the new "genpd_alloc_default_state()" instead.

drivers/base/power/domain.c void pm_genpd_init(struct generic_pm_domain *genpd, · line 1591

Quoted 6 lines

drivers/base/power/domain.c +6 -0

+               if (ret)
+                       return;
+       }
+
+       /* Assume the deepest state on init*/
+       genpd->state_idx = genpd->state_count - 1;

When the PM domain is powered on, in other words the genpd->status == GPD_STATE_ACTIVE, I guess this value doesn't matter much.

Although, what if the PM domain is powered off, don't you need to respect the value that might have been assigned by the SoC PM domain driver? Else you may at the next power on, indicate that you are coming from a state that's not the correct one.

Perhaps, you should only set genpd->state_idx = 0, from genpd_alloc_default_state() and in the other case leave the value as is?

Quoted 76 lines

drivers/base/power/domain.c +41 -1

+
        mutex_lock(&gpd_list_lock);
        list_add(&genpd->gpd_list_node, &gpd_list);
        mutex_unlock(&gpd_list_lock);

@@ -1822,6 +1909,33 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
 #include <linux/kobject.h>
 static struct dentry *pm_genpd_debugfs_dir;
 
+static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+                                      const struct genpd_power_state *st,
+                                      unsigned int st_count)
+{
+       unsigned int i;
+
+       if (IS_ERR_OR_NULL(genpd))
+               return -EINVAL;
+
+       if (genpd->state_count != st_count) {
+               pr_err("Invalid allocated state count\n");
+               return -EINVAL;
+       }
+
+       for (i = 0; i < st_count; i++) {
+               genpd->states[i].name = kstrndup(st[i].name,
+                               GENPD_MAX_NAME_SIZE, GFP_KERNEL);
+               if (!genpd->states[i].name) {
+                       pr_err("%s Failed to allocate state %d name.\n",
+                               genpd->name, i);
+                       return -ENOMEM;
+               }
+       }
+
+       return 0;
+}
+
 /*
  * TODO: This function is a slightly modified version of rtpm_status_show
  * from sysfs.c, so generalize it.

@@ -1855,6 +1969,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
                [GPD_STATE_ACTIVE] = "on",
                [GPD_STATE_POWER_OFF] = "off"
        };
+       unsigned int state_idx = genpd->state_idx;
        struct pm_domain_data *pm_data;
        const char *kobj_path;
        struct gpd_link *link;

@@ -1866,7 +1981,11 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
        if (WARN_ON(genpd->status >= ARRAY_SIZE(status_lookup)))
                goto exit;
-       seq_printf(s, "%-30s  %-15s ", genpd->name, status_lookup[genpd->status]);
+
+       seq_printf(s, "%-30s  %-15s  ", genpd->name,
+                  (genpd->status == GPD_STATE_POWER_OFF) ?
+                  genpd->states[state_idx].name :
+                  status_lookup[genpd->status]);
 
        /*
         * Modifications on the list require holding locks on both

@@ -1954,4 +2073,11 @@ static void __exit pm_genpd_debug_exit(void)
        debugfs_remove_recursive(pm_genpd_debugfs_dir);
 }
 __exitcall(pm_genpd_debug_exit);
+#else
+static inline int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
+                                       const struct genpd_power_state *st,
+                                       unsigned int st_count)
+{
+       return 0;
+}
 #endif /* CONFIG_PM_ADVANCED_DEBUG */

drivers/base/power/domain_governor.c +0 -0

index e60dd12..b4984f5 100644

drivers/base/power/domain_governor.c static bool default_power_down_ok(struct dev_pm_domain *pd) · line 131

Quoted 9 lines

drivers/base/power/domain_governor.c +4 -2

@@ -125,8 +125,12 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
                return genpd->cached_power_down_ok;
        }
 
-       off_on_time_ns = genpd->power_off_latency_ns +
-                               genpd->power_on_latency_ns;
+       /*
+        * Use the only available state, until multiple state support is added
+        * to the governor.
+        */

So you want to delay this step into a later patch. I am fine with that, but you need to state that in the change-log.

Perhaps it's better to say that we will always try with the shallowest/deepest off state, and just ignore the other states in this phase?

drivers/base/power/domain_governor.c static bool default_power_down_ok(struct dev_pm_domain *pd) · line 133

Quoted 2 lines

drivers/base/power/domain_governor.c +2 -0

+       off_on_time_ns = genpd->states[0].power_off_latency_ns +
+                               genpd->states[0].power_on_latency_ns;

If you think my above idea seems reasonable, that should change the above to:

off_on_time_ns = genpd->states[genpd->state_count - 1].power_off_latency_ns +

     genpd->states[genpd->state_count - 1].power_on_latency_ns;

drivers/base/power/domain_governor.c static bool default_power_down_ok(struct dev_pm_domain *pd) · line 214

Quoted 13 lines

drivers/base/power/domain_governor.c +4 -1

        min_off_time_ns = -1;
        /*

@@ -203,8 +207,11 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
         * The difference between the computed minimum subdomain or device off
         * time and the time needed to turn the domain on is the maximum
         * theoretical time this domain can spend in the "off" state.
+        * Use the only available state, until multiple state support is added
+        * to the governor.
         */
-       genpd->max_off_time_ns = min_off_time_ns - genpd->power_on_latency_ns;
+       genpd->max_off_time_ns = min_off_time_ns -
+               genpd->states[0].power_on_latency_ns;

Ditto.

include/linux/pm_domain.h struct gpd_dev_ops { · line 41

Quoted 13 lines

drivers/base/power/domain_governor.c +0 -0

        return true;
 }
 

include/linux/pm_domain.h +2 -0

index ba4ced3..11763cf 100644
@@ -37,6 +37,12 @@ struct gpd_dev_ops {
        bool (*active_wakeup)(struct device *dev);
 };
 
+struct genpd_power_state {
+       char *name;

As stated, suggest to remove "name".

Quoted 19 lines

include/linux/pm_domain.h +8 -1

+       s64 power_off_latency_ns;
+       s64 power_on_latency_ns;
+};
+
 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 */

@@ -66,6 +72,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;
+       unsigned int state_count; /* number of states */
+       unsigned int state_idx; /* state that genpd will go to when off */
+
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
--

Quoted 2 lines

2.1.4

Kind regards
Uffe

Quoted 7 lines

Quoted 1 line

+static int genpd_alloc_states_data(struct generic_pm_domain *genpd,

In this patch I would rather just add *one* new "alloc" function and name it like below.

static int genpd_alloc_default_state(struct generic_pm_domain *genpd)

Quoted 2 lines

I assume the name I suggest for it, indicates what it needs to do and *not* needs to do.

Hi Ulf,

Thanks for your thorough review!
i will implement your suggestions for the next spin,

However I have a doubt about this comment, do you mean that i should get rid of genpd_alloc_states_data completly? i had a static number of states before

but that was droped because o wasted memory if no states were needed.

if you just meant that it should be added in a separate patch, since i will be sqashing

this patch with "PM / Domains: make governor select deepest state" can
i keep it here? maybe it should go with the dt parsing patch from Marc?

From: Marc Titinger <[email protected]>

From: Marc Titinger <[email protected]>

This patch allows cluster-level idle-states to being soaked in as generic domain power states, in order for the domain governor to chose the most efficient power state compatible with the device constraints.

Similarly, devices can register power-states into the cluster domain, in a manner consistent with idle-states.

This is a attempt to address device-retention states for devices that are not hooked to runtime-pm, but feature a retention state handled by the same firmware that handles idle-states. For instance a L2 caches.

With Juno, in this example the idle-state 'cluster-sleep-0 ' is known from each cluster generic domain, as the deepest sate.

cat /sys/kernel/debug/pm_genpd/*

Domain State name Enter (ns) / Exit (ns)
-------------------------------------------------------------

a53_pd               cluster-sleep-0      1500000 / 800000
a57_pd               cluster-sleep-0      1500000 / 800000

domain status pstate slaves
/device runtime status
-----------------------------------------------------------------------
a53_pd on

/devices/system/cpu/cpu0                            active
/devices/system/cpu/cpu3                            suspended
/devices/system/cpu/cpu4                            suspended
/devices/system/cpu/cpu5                            suspended
/devices/platform/D1                                suspended

a57_pd cluster-sleep-0
/devices/system/cpu/cpu1 suspended
/devices/system/cpu/cpu2 suspended

Signed-off-by Marc Titinger <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: removed dependency on dynamic states, simplified initalization, added of_pm_genpd_init() API]

---
 .../devicetree/bindings/power/power_domain.txt     |  63 ++++++++++

 drivers/base/power/domain.c                        | 128 +++++++++++++++++++++
 include/linux/pm_domain.h                          |   6 +
 3 files changed, 197 insertions(+)

Documentation/devicetree/bindings/power/power_domain.txt +0 -0

index 025b5e7..e2f542e 100644

Documentation/devicetree/bindings/power/power_domain.txt +14 -0

+
+       - entry-latency-us
+               Usage: Not required if wakeup-latency-us is provided.
+               Value type: <prop-encoded-array>
+               Definition: u32 value representing worst case latency in
+               microseconds required to enter the idle state.
+               The exit-latency-us duration may be guaranteed
+               only after entry-latency-us has passed.
+
+       - exit-latency-us
+               Usage: Not required if wakeup-latency-us is provided.
+               Value type: <prop-encoded-array>
+               Definition: u32 value representing worst case latency
+               in microseconds required to exit the idle state.

Documentation/devicetree/bindings/power/power_domain.txt +13 -0

+
 Example:
 
        power: power-controller at 12340000 {

@@ -55,6 +93,31 @@ Example 2:
                #power-domain-cells = <1>;
        };
 
+Example 3:
+
+       pm-domains {
+               a57_pd: a57_pd@ {
+                       /* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
+                       compatible = "arm,pd","arm,cortex-a57";
+                       #power-domain-cells = <0>;
+                       power-states = <&CLUSTER_SLEEP_0>;
+               };
+
+               a53_pd: a53_pd@ {
+                       /* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/

Documentation/devicetree/bindings/power/power_domain.txt +3 -0

+       };
+
+
 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.

drivers/base/power/domain.c +0 -0

index 3242854..fe1be88 100644

drivers/base/power/domain.c +127 -0

 static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
                                       const struct genpd_power_state *st,

@@ -1515,6 +1516,105 @@ static int pm_genpd_default_restore_state(struct device *dev)
        return cb ? cb(dev) : 0;
 }
 
+static const struct of_device_id power_state_match[] = {
+       { .compatible = "linux,domain-state" },
+       { }
+};
+
+static int of_get_genpd_power_state(struct genpd_power_state *genpd_state,
+                                          struct device_node *state_node)
+{
+       const struct of_device_id *match_id;
+       int err = 0;
+       u32 latency;
+
+       match_id = of_match_node(power_state_match, state_node);
+       if (!match_id)
+               return -ENODEV;
+
+       err = of_property_read_u32(state_node, "wakeup-latency-us", &latency);
+       if (err) {
+               u32 entry_latency, exit_latency;
+
+               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;
+               }
+               /*
+                * If wakeup-latency-us is missing, default to entry+exit
+                * latencies as defined in idle states bindings
+                */
+               latency = entry_latency + exit_latency;
+       }
+
+       genpd_state->power_on_latency_ns = 1000 * latency;
+
+       err = of_property_read_u32(state_node, "entry-latency-us", &latency);
+       if (err) {
+               pr_debug(" * %s missing min-residency-us property\n",
+                        state_node->full_name);
+               return -EINVAL;
+       }
+
+       genpd_state->power_off_latency_ns = 1000 * latency;
+
+       return 0;
+}
+
+static int of_genpd_device_parse_states(struct device_node *np,
+                                struct genpd_power_state *genpd_states,
+                                int *state_count)
+{
+       struct device_node *state_node;
+       int i, err = 0;
+
+       for (i = 0;; i++) {
+               struct genpd_power_state genpd_state;
+
+               state_node = of_parse_phandle(np, "power-states", i);
+               if (!state_node)
+                       break;
+
+               if (i == GENPD_MAX_DOMAIN_STATES) {
+                       err = -ENOMEM;
+                       break;
+               }
+
+               err = of_get_genpd_power_state(&genpd_state, state_node);
+               if (err) {
+                       pr_err
+                           ("Parsing idle state node %s failed with err %d\n",
+                            state_node->full_name, err);
+                       err = -EINVAL;
+                       break;
+               }
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+               genpd_state.name = kstrndup(state_node->full_name,
+                                           GENPD_MAX_NAME_SIZE, GFP_KERNEL);
+               if (!genpd_state.name)
+                       err = -ENOMEM;
+#endif
+               of_node_put(state_node);
+               memcpy(&genpd_states[i], &genpd_state, sizeof(genpd_state));
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+               kfree(genpd_state.name);
+#endif
+       }
+       *state_count = i;
+       return err;
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.

@@ -1596,6 +1696,34 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+int of_pm_genpd_init(struct device_node *dn, struct generic_pm_domain *genpd,
+                  struct dev_power_governor *gov, bool is_off)
+{
+       struct genpd_power_state states[GENPD_MAX_DOMAIN_STATES] = { { 0 } };
+       int state_count = GENPD_MAX_DOMAIN_STATES;
+       int ret;
+
+       if (IS_ERR_OR_NULL(genpd))
+               return -EINVAL;
+
+       ret = of_genpd_device_parse_states(dn, states, &state_count);
+       if (ret) {
+               pr_err("Error parsing genpd states for %s\n", genpd->name);
+               return ret;
+       }
+
+       ret = genpd_alloc_states_data(genpd, states, state_count);
+       if (ret) {
+               pr_err("Failed to allocate states for %s\n", genpd->name);
+               return ret;
+       }
+
+       pm_genpd_init(genpd, gov, is_off);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(of_pm_genpd_init);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.

include/linux/pm_domain.h +6 -1

index 11763cf..e425911 100644
@@ -213,6 +213,8 @@ struct generic_pm_domain *__of_genpd_xlate_onecell(
                                        void *data);
 
 int genpd_dev_pm_attach(struct device *dev);
+int of_pm_genpd_init(struct device_node *dn, struct generic_pm_domain *genpd,
+                  struct dev_power_governor *gov, bool is_off);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
 static inline int __of_genpd_add_provider(struct device_node *np,
                                        genpd_xlate_t xlate, void *data)

@@ -234,6 +236,10 @@ static inline int genpd_dev_pm_attach(struct device *dev)
 {
        return -ENODEV;
 }
+
+static inline int of_pm_genpd_init(struct device_node *dn,
+               struct generic_pm_domain *genpd,
+               struct dev_power_governor *gov, bool is_off) {}
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 static inline int of_genpd_add_provider_simple(struct device_node *np,
--

2.1.4

Quoted 7 lines · Lina Iyer

From: Marc Titinger <[email protected]>

From: Marc Titinger <[email protected]>

This patch allows cluster-level idle-states to being soaked in as generic domain power states, in order for the domain governor to chose the most efficient power state compatible with the device constraints.

That was possible before this patch, although you are now making it possible to describe this in DT and also modifying genpd to parse this information.

Also, I don't get what this has to do with the governor. Of course that governor needs to care about the multiple states you added in patch1, but it should do that no matter if the information comes from DT or not, right.

Please re-phrase this to make it bit more clear.

Quoted 3 lines

Similarly, devices can register power-states into the cluster domain, in a manner consistent with idle-states.

I don't get this. Can you please elaborate?

Quoted 5 lines

This is a attempt to address device-retention states for devices that are not hooked to runtime-pm, but feature a retention state handled by the same firmware that handles idle-states. For instance a L2 caches.

I guess whether devices may use runtime PM or not, they still can be attached to a PM domain with multiple power states?

Quoted 25 lines

With Juno, in this example the idle-state 'cluster-sleep-0 ' is known from each cluster generic domain, as the deepest sate.

cat /sys/kernel/debug/pm_genpd/*

Domain State name Enter (ns) / Exit (ns)
-------------------------------------------------------------

a53_pd               cluster-sleep-0      1500000 / 800000
a57_pd               cluster-sleep-0      1500000 / 800000

domain status pstate slaves
/device runtime status
-----------------------------------------------------------------------
a53_pd on

/devices/system/cpu/cpu0                            active
/devices/system/cpu/cpu3                            suspended
/devices/system/cpu/cpu4                            suspended
/devices/system/cpu/cpu5                            suspended
/devices/platform/D1                                suspended

a57_pd cluster-sleep-0
/devices/system/cpu/cpu1 suspended
/devices/system/cpu/cpu2 suspended

Quoted 6 lines

Signed-off-by Marc Titinger <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: removed dependency on dynamic states, simplified initalization, added of_pm_genpd_init() API]

---
  .../devicetree/bindings/power/power_domain.txt     |  63 ++++++++++

Please split the documentation of the suggested new DT bindings into a separate patch that is preceding this one.

Also, you need to send it to the DT maintainers.

Quoted 12 lines

  drivers/base/power/domain.c                        | 128 +++++++++++++++++++++
  include/linux/pm_domain.h                          |   6 +
  3 files changed, 197 insertions(+)

Documentation/devicetree/bindings/power/power_domain.txt +0 -0

index 025b5e7..e2f542e 100644
@@ -29,6 +29,44 @@ Optional properties:
     specified by this binding. More details about power domain specifier are
     available in the next section.
 

Documentation/devicetree/bindings/power/power_domain.txt Optional properties: · line 34

Quoted 3 lines

Documentation/devicetree/bindings/power/power_domain.txt +3 -0

+- power-states : A phandle of an idle-state that shall be soaked into a
+                generic domain power state. The power-state shall be
+               compatible with "linux,domain-state".

Why mention the Linux specific "generic power domain" here?

Let's instead try to describe this without using specific terminology from Linux.

Documentation/devicetree/bindings/power/power_domain.txt Optional properties: · line 44

Quoted 11 lines

Documentation/devicetree/bindings/power/power_domain.txt +10 -0

+
+==Power state==
+
+A PM domain power state describes an idle state of a domain and must be
+have the following properties -
+
+       - compatible
+               Usage: Required
+               Value type: <stringlist>
+               Definition: Must be "linux,domain-state"

Why do you need a compatible string?

You already have the phandle available, I suppose you can use that instead of parsing for a new compatible string.

Quoted 19 lines

Documentation/devicetree/bindings/power/power_domain.txt +19 -0

+
+       - entry-latency-us
+               Usage: Not required if wakeup-latency-us is provided.
+               Value type: <prop-encoded-array>
+               Definition: u32 value representing worst case latency in
+               microseconds required to enter the idle state.
+               The exit-latency-us duration may be guaranteed
+               only after entry-latency-us has passed.
+
+       - exit-latency-us
+               Usage: Not required if wakeup-latency-us is provided.
+               Value type: <prop-encoded-array>
+               Definition: u32 value representing worst case latency
+               in microseconds required to exit the idle state.
+
+       - wakeup-latency-us:
+               Usage: Not required if entry-latency-us and exit-latency-us
+               are provided.
+               Value type: <prop-encoded-array>

Documentation/devicetree/bindings/power/power_domain.txt Optional properties: · line 68

Quoted 5 lines

Documentation/devicetree/bindings/power/power_domain.txt +5 -0

+               Definition: u32 value representing maximum delay between the
+               signaling the wakeup of the domain and the domain resuming
+               regular operation.
+               If omitted, this is assumed to be equal to:
+                       entry-latency-us + exit-latency-us

I think we should decide which of the two alternatives to use, we shouldn't make it more complicated than needed.

Quoted 21 lines

Documentation/devicetree/bindings/power/power_domain.txt +13 -0

+
  Example:
 
         power: power-controller at 12340000 {

@@ -55,6 +93,31 @@ Example 2:
                 #power-domain-cells = <1>;
         };
 
+Example 3:
+
+       pm-domains {
+               a57_pd: a57_pd@ {
+                       /* will have a57 platform ARM_PD_METHOD_OF_DECLARE*/
+                       compatible = "arm,pd","arm,cortex-a57";
+                       #power-domain-cells = <0>;
+                       power-states = <&CLUSTER_SLEEP_0>;
+               };
+
+               a53_pd: a53_pd@ {
+                       /* will have a a53 platform ARM_PD_METHOD_OF_DECLARE*/

Documentation/devicetree/bindings/power/power_domain.txt Example 2: · line 117

Quoted 10 lines

Documentation/devicetree/bindings/power/power_domain.txt +10 -0

+                       compatible = "arm,pd","arm,cortex-a53";
+                       #power-domain-cells = <0>;
+                       power-states = <&CLUSTER_SLEEP_0>;
+               };
+
+               CLUSTER_SLEEP_0: power-state at 0 {
+                       compatible = "pm-domain,power-state";
+                       entry-latency-us = <1000>;
+                       exit-latency-us = <2000>;
+               };

I think we should also cover the case when power-controller has "#power-domain-cells = <[n>0]>".

Perhaps extending the first example is then better than adding a new one!?

Quoted 11 lines

Documentation/devicetree/bindings/power/power_domain.txt +3 -0

+       };
+
+
  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.

drivers/base/power/domain.c +0 -0

index 3242854..fe1be88 100644

drivers/base/power/domain.c line 38

Quoted 4 lines

drivers/base/power/domain.c +1 -0

@@ -35,6 +35,7 @@
  })
 
  #define GENPD_MAX_NAME_SIZE 20
+#define GENPD_MAX_DOMAIN_STATES 10

Instead of setting a hard limit, let's pre-parse the DTB to get the number of power states. In that way, it should be fairly easy to convert the below code to be more dynamic.

Quoted 169 lines

drivers/base/power/domain.c +127 -0

  static int pm_genpd_alloc_states_names(struct generic_pm_domain *genpd,
                                        const struct genpd_power_state *st,

@@ -1515,6 +1516,105 @@ static int pm_genpd_default_restore_state(struct device *dev)
         return cb ? cb(dev) : 0;
  }
 
+static const struct of_device_id power_state_match[] = {
+       { .compatible = "linux,domain-state" },
+       { }
+};
+
+static int of_get_genpd_power_state(struct genpd_power_state *genpd_state,
+                                          struct device_node *state_node)
+{
+       const struct of_device_id *match_id;
+       int err = 0;
+       u32 latency;
+
+       match_id = of_match_node(power_state_match, state_node);
+       if (!match_id)
+               return -ENODEV;
+
+       err = of_property_read_u32(state_node, "wakeup-latency-us", &latency);
+       if (err) {
+               u32 entry_latency, exit_latency;
+
+               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;
+               }
+               /*
+                * If wakeup-latency-us is missing, default to entry+exit
+                * latencies as defined in idle states bindings
+                */
+               latency = entry_latency + exit_latency;
+       }
+
+       genpd_state->power_on_latency_ns = 1000 * latency;
+
+       err = of_property_read_u32(state_node, "entry-latency-us", &latency);
+       if (err) {
+               pr_debug(" * %s missing min-residency-us property\n",
+                        state_node->full_name);
+               return -EINVAL;
+       }
+
+       genpd_state->power_off_latency_ns = 1000 * latency;
+
+       return 0;
+}
+
+static int of_genpd_device_parse_states(struct device_node *np,
+                                struct genpd_power_state *genpd_states,
+                                int *state_count)
+{
+       struct device_node *state_node;
+       int i, err = 0;
+
+       for (i = 0;; i++) {
+               struct genpd_power_state genpd_state;
+
+               state_node = of_parse_phandle(np, "power-states", i);
+               if (!state_node)
+                       break;
+
+               if (i == GENPD_MAX_DOMAIN_STATES) {
+                       err = -ENOMEM;
+                       break;
+               }
+
+               err = of_get_genpd_power_state(&genpd_state, state_node);
+               if (err) {
+                       pr_err
+                           ("Parsing idle state node %s failed with err %d\n",
+                            state_node->full_name, err);
+                       err = -EINVAL;
+                       break;
+               }
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+               genpd_state.name = kstrndup(state_node->full_name,
+                                           GENPD_MAX_NAME_SIZE, GFP_KERNEL);
+               if (!genpd_state.name)
+                       err = -ENOMEM;
+#endif
+               of_node_put(state_node);
+               memcpy(&genpd_states[i], &genpd_state, sizeof(genpd_state));
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+               kfree(genpd_state.name);
+#endif
+       }
+       *state_count = i;
+       return err;
+}
+
  /**
   * pm_genpd_init - Initialize a generic I/O PM domain object.
   * @genpd: PM domain object to initialize.

@@ -1596,6 +1696,34 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
  }
  EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+int of_pm_genpd_init(struct device_node *dn, struct generic_pm_domain *genpd,
+                  struct dev_power_governor *gov, bool is_off)
+{
+       struct genpd_power_state states[GENPD_MAX_DOMAIN_STATES] = { { 0 } };
+       int state_count = GENPD_MAX_DOMAIN_STATES;
+       int ret;
+
+       if (IS_ERR_OR_NULL(genpd))
+               return -EINVAL;
+
+       ret = of_genpd_device_parse_states(dn, states, &state_count);
+       if (ret) {
+               pr_err("Error parsing genpd states for %s\n", genpd->name);
+               return ret;
+       }
+
+       ret = genpd_alloc_states_data(genpd, states, state_count);
+       if (ret) {
+               pr_err("Failed to allocate states for %s\n", genpd->name);
+               return ret;
+       }
+
+       pm_genpd_init(genpd, gov, is_off);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(of_pm_genpd_init);
+
  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
  /*
   * Device Tree based PM domain providers.

include/linux/pm_domain.h +6 -1

index 11763cf..e425911 100644
@@ -213,6 +213,8 @@ struct generic_pm_domain *__of_genpd_xlate_onecell(
                                         void *data);
 
  int genpd_dev_pm_attach(struct device *dev);
+int of_pm_genpd_init(struct device_node *dn, struct generic_pm_domain *genpd,
+                  struct dev_power_governor *gov, bool is_off);
  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
  static inline int __of_genpd_add_provider(struct device_node *np,
                                         genpd_xlate_t xlate, void *data)

@@ -234,6 +236,10 @@ static inline int genpd_dev_pm_attach(struct device *dev)
  {
         return -ENODEV;
  }
+
+static inline int of_pm_genpd_init(struct device_node *dn,
+               struct generic_pm_domain *genpd,
+               struct dev_power_governor *gov, bool is_off) {}
  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
  static inline int of_genpd_add_provider_simple(struct device_node *np,
--

Quoted 2 lines

2.1.4

Instead of commenting directly to the changes above, let me instead raise a couple of concerns for the overall approach.

1)
The case when the power-controller node has "#power-domain-cells = <[n>0]>", isn't covered by $subject patch. I guess we should fix that!?

2)
I think that it needs to be the SOC PM domain driver that assigns the initial value for "genpd->state_idx", as I stated also for patch1 when I noticed pm_genpd_init() overrides that value. dev_pm_domain_attach
If you share my view around this, it also means that the parsing of the DTB for "power-states" needs to be done prior the SOC PM domain driver assigns this initial value for "genpd->state_idx". Following your approach in $subject patch, means that its value needs to be updated *after* pm_genpd_init() have been called, perhaps not ideal.

Can we find a better way to deal with this? Maybe by instead of adding the of_pm_genpd_init() API, we should provide APIs which manages the DT parsing and the allocation of the struct genpd_power_state? Just an idea though.

Kind regards
Uffe

Quoted 16 lines · Ulf Hansson

Documentation/devicetree/bindings/power/power_domain.txt Optional properties: · line 34

Quoted 11 lines · Lina Iyer

Documentation/devicetree/bindings/power/power_domain.txt +3 -0

index 025b5e7..e2f542e 100644
@@ -29,6 +29,44 @@ Optional properties:
    specified by this binding. More details about power domain specifier are
    available in the next section.
 
+- power-states : A phandle of an idle-state that shall be soaked into a
+                generic domain power state. The power-state shall be
+               compatible with "linux,domain-state".

Why mention the Linux specific "generic power domain" here?

Quoted 2 lines

Let's instead try to describe this without using specific terminology from Linux.

Hmm, I will need Lina's help to answer this one. Following a previous review actually I dropped this power-states compatible, to stick with idle-states.

Quoted 14 lines

Documentation/devicetree/bindings/power/power_domain.txt Optional properties: · line 44

Quoted 11 lines

Documentation/devicetree/bindings/power/power_domain.txt +10 -0

+
+==Power state==
+
+A PM domain power state describes an idle state of a domain and must be
+have the following properties -
+
+       - compatible
+               Usage: Required
+               Value type: <stringlist>
+               Definition: Must be "linux,domain-state"

Why do you need a compatible string?

Quoted 3 lines

You already have the phandle available, I suppose you can use that instead of parsing for a new compatible string.

Quoted 19 lines

Quoted 19 lines

Documentation/devicetree/bindings/power/power_domain.txt +19 -0

+
+       - entry-latency-us
+               Usage: Not required if wakeup-latency-us is provided.
+               Value type: <prop-encoded-array>
+               Definition: u32 value representing worst case latency in
+               microseconds required to enter the idle state.
+               The exit-latency-us duration may be guaranteed
+               only after entry-latency-us has passed.
+
+       - exit-latency-us
+               Usage: Not required if wakeup-latency-us is provided.
+               Value type: <prop-encoded-array>
+               Definition: u32 value representing worst case latency
+               in microseconds required to exit the idle state.
+
+       - wakeup-latency-us:
+               Usage: Not required if entry-latency-us and exit-latency-us
+               are provided.
+               Value type: <prop-encoded-array>

Quoted 8 lines

Documentation/devicetree/bindings/power/power_domain.txt Optional properties: · line 68

Quoted 5 lines

Documentation/devicetree/bindings/power/power_domain.txt +5 -0

+               Definition: u32 value representing maximum delay between the
+               signaling the wakeup of the domain and the domain resuming
+               regular operation.
+               If omitted, this is assumed to be equal to:
+                       entry-latency-us + exit-latency-us

I think we should decide which of the two alternatives to use, we shouldn't make it more complicated than needed.

Agreed.

Quoted 6 lines · Ulf Hansson

Quoted 2 lines · Lina Iyer

Similarly, devices can register power-states into the cluster domain, in a manner consistent with idle-states.

I don't get this. Can you please elaborate?

Alexes addition of "power states" to the power domains was to have a better representation of features of power controllers. For instance QoS may prevent to enter/exit complete power-off, but setting the core voltage to say 0.7v is feasible in terms of timing, and still better than full-power-on etc... Hence the domain has a list of possible states to chose upon, between full power-on and full-power-off, and genpd will call the platform code for this.

Now, this patch maps the idle-states as possible power states for the
domain: upon the last CPU runtime_put, the domain can chose the deepest
state that can be reached given the enter/exit time available, and call the platform code for this. This selected state can be any of:
* cluster-sleep (power OFF)
* cluster-retention A (L2RAM retention for instance)
* cluster-retention B (some device is still on, like PMU or bus analyzer, healthckeck IP etc...)
...
* cluster-on, but lower voltage A
* cluster-on, but lower voltage B
etc...

Put short: CPUs, like any other devices in the domain may register a power state.

Quoted 6 lines

Quoted 5 lines

This is a attempt to address device-retention states for devices that are not hooked to runtime-pm, but feature a retention state handled by the same firmware that handles idle-states. For instance a L2 caches.

Quoted 2 lines

I guess whether devices may use runtime PM or not, they still can be attached to a PM domain with multiple power states?

yes.

From what I understand, it seems like you want to have a constraint on a

Allow domain states to hold additional state related data in a u32 value. This may be used by the platform driver.

From: Axel Haslam <[email protected]>

Now that the structures of genpd can support multiple state definitions, add the logic in the governor to select the deepest possible state when powering down.

For this, create the new function power_down_ok_for_state which will test if a particular state will not violate the devices and sub-domains constraints.

default_power_down_ok is modified to try each state starting from the deepest until a valid state is found or there are no more states to test.

the resulting state will be valid until there are latency or constraint changes, thus, we can avoid looping every power_down, and use the cached results instead.

Signed-off-by Axel Haslam <[email protected]>

---
 drivers/base/power/domain_governor.c | 75 +++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 30 deletions(-)

drivers/base/power/domain_governor.c +5 -29

 {
        struct generic_pm_domain *genpd = pd_to_genpd(pd);
        struct gpd_link *link;

@@ -106,31 +107,9 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
        s64 min_off_time_ns;
        s64 off_on_time_ns;
 
-       if (genpd->max_off_time_changed) {
-               struct gpd_link *link;
+       off_on_time_ns = genpd->states[state].power_off_latency_ns +
+               genpd->states[state].power_on_latency_ns;
 
-               /*
-                * We have to invalidate the cached results for the masters, so
-                * use the observation that default_power_down_ok() is not
-                * going to be called for any master until this instance
-                * returns.
-                */
-               list_for_each_entry(link, &genpd->slave_links, slave_node)
-                       link->master->max_off_time_changed = true;
-
-               genpd->max_off_time_changed = false;
-               genpd->cached_power_down_ok = false;
-               genpd->max_off_time_ns = -1;
-       } else {
-               return genpd->cached_power_down_ok;
-       }
-
-       /*
-        * Use the only available state, until multiple state support is added
-        * to the governor.
-        */
-       off_on_time_ns = genpd->states[0].power_off_latency_ns +
-                               genpd->states[0].power_on_latency_ns;
 
        min_off_time_ns = -1;
        /*

@@ -193,8 +172,6 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
                        min_off_time_ns = constraint_ns;
        }
 
-       genpd->cached_power_down_ok = true;
-
        /*
         * If the computed minimum device off time is negative, there are no
         * latency constraints, so the domain can spend arbitrary time in the

@@ -207,14 +184,52 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
         * The difference between the computed minimum subdomain or device off
         * time and the time needed to turn the domain on is the maximum
         * theoretical time this domain can spend in the "off" state.
-        * Use the only available state, until multiple state support is added
-        * to the governor.
         */
        genpd->max_off_time_ns = min_off_time_ns -
-               genpd->states[0].power_on_latency_ns;
+                       genpd->states[state].power_on_latency_ns;
        return true;
 }
 
+static bool default_power_down_ok(struct dev_pm_domain *pd)
+{

drivers/base/power/domain_governor.c +6 -0

+       if (!genpd->max_off_time_changed)
+               return genpd->cached_power_down_ok;
+
+       /*
+        * We have to invalidate the cached results for the masters, so
+        * use the observation that default_power_down_ok() is not

drivers/base/power/domain_governor.c +5 -1

+
+       genpd->cached_power_down_ok = retval;
+       return retval;
+}
+
 static bool always_on_power_down_ok(struct dev_pm_domain *domain)
 {
        return false;
--

2.1.4

From: Axel Haslam <[email protected]>

The generic_pm_domain structure uses an array of latencies to be able to declare multiple intermediate states.

Declare a single "OFF" state with the default latencies So that the power_off_latency_ns and power_on_latency_ns fields of generic_pm_domain structure can be eventualy removed.

Signed-off-by Axel Haslam <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: pm_genpd_init() argument change]

---
 arch/arm/mach-imx/gpc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm/mach-imx/gpc.c +0 -0

index 8e7976a..b070e06 100644

arch/arm/mach-imx/gpc.c +6 -0

+	{
+		.name = "OFF",
+		.power_off_latency_ns = 25000,
+		.power_on_latency_ns = 2000000,
+	},
+};

Am Mittwoch, den 18.11.2015, 07:57 -0700 schrieb Lina Iyer:

Quoted 21 lines

From: Axel Haslam <[email protected]>

The generic_pm_domain structure uses an array of latencies to be able to declare multiple intermediate states.

Declare a single "OFF" state with the default latencies So that the power_off_latency_ns and power_on_latency_ns fields of generic_pm_domain structure can be eventualy removed.

Signed-off-by Axel Haslam <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: pm_genpd_init() argument change]

---
 arch/arm/mach-imx/gpc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm/mach-imx/gpc.c +0 -0

index 8e7976a..b070e06 100644

arch/arm/mach-imx/gpc.c static struct generic_pm_domain imx6q_arm_domain = { · line 371

Quoted 4 lines

arch/arm/mach-imx/gpc.c +1 -0

@@ -368,13 +368,23 @@ static struct generic_pm_domain imx6q_arm_domain = {
 	.name = "ARM",
 };
 
+static struct genpd_power_state imx6q_arm_domain_states[] = {

Those states are for the PU, not the ARM domain.

Quoted 6 lines

arch/arm/mach-imx/gpc.c +6 -0

+	{
+		.name = "OFF",
+		.power_off_latency_ns = 25000,
+		.power_on_latency_ns = 2000000,
+	},
+};

arch/arm/mach-imx/gpc.c static struct generic_pm_domain imx6q_arm_domain = { · line 385

Quoted 10 lines

arch/arm/mach-imx/gpc.c +3 -2

+
 static struct pu_domain imx6q_pu_domain = {
 	.base = {
 		.name = "PU",
 		.power_off = imx6q_pm_pu_power_off,
 		.power_on = imx6q_pm_pu_power_on,
-		.power_off_latency_ns = 25000,
-		.power_on_latency_ns = 2000000,
+		.gov = NULL,
+		.status = GPD_STATE_POWER_OFF,

The above 2 lines should not be needed. This is a static struct, so .gov will already be NULL. status is set by pm_genpd_init() and the above value is actively wrong.

arch/arm/mach-imx/gpc.c static struct generic_pm_domain imx6q_arm_domain = { · line 390

Quoted 5 lines

arch/arm/mach-imx/gpc.c +2 -0

+		.states = imx6q_arm_domain_states,
+		.state_count = ARRAY_SIZE(imx6q_arm_domain_states),
 	},
 };
 

How urgent is this patch? I have a series to rework the GPC driver almost ready and I could fold this change in directly if it's okay for this to sit through a review of the rework.

Am Montag, den 23.11.2015, 14:31 +0100 schrieb Lucas Stach:

Quoted 50 lines

Am Mittwoch, den 18.11.2015, 07:57 -0700 schrieb Lina Iyer:

Quoted 21 lines

From: Axel Haslam <[email protected]>

The generic_pm_domain structure uses an array of latencies to be able to declare multiple intermediate states.

Declare a single "OFF" state with the default latencies So that the power_off_latency_ns and power_on_latency_ns fields of generic_pm_domain structure can be eventualy removed.

Signed-off-by Axel Haslam <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Lina: pm_genpd_init() argument change]

---
 arch/arm/mach-imx/gpc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/arm/mach-imx/gpc.c +0 -0

index 8e7976a..b070e06 100644

arch/arm/mach-imx/gpc.c static struct generic_pm_domain imx6q_arm_domain = { · line 371

Quoted 4 lines

arch/arm/mach-imx/gpc.c +1 -0

@@ -368,13 +368,23 @@ static struct generic_pm_domain imx6q_arm_domain = {
 	.name = "ARM",
 };
 
+static struct genpd_power_state imx6q_arm_domain_states[] = {

Those states are for the PU, not the ARM domain.

Quoted 6 lines

arch/arm/mach-imx/gpc.c +6 -0

+	{
+		.name = "OFF",
+		.power_off_latency_ns = 25000,
+		.power_on_latency_ns = 2000000,
+	},
+};

arch/arm/mach-imx/gpc.c static struct generic_pm_domain imx6q_arm_domain = { · line 385

Quoted 10 lines

arch/arm/mach-imx/gpc.c +3 -2

+
 static struct pu_domain imx6q_pu_domain = {
 	.base = {
 		.name = "PU",
 		.power_off = imx6q_pm_pu_power_off,
 		.power_on = imx6q_pm_pu_power_on,
-		.power_off_latency_ns = 25000,
-		.power_on_latency_ns = 2000000,
+		.gov = NULL,
+		.status = GPD_STATE_POWER_OFF,

The above 2 lines should not be needed. This is a static struct, so .gov will already be NULL. status is set by pm_genpd_init() and the above value is actively wrong.

Quoted 9 lines

arch/arm/mach-imx/gpc.c static struct generic_pm_domain imx6q_arm_domain = { · line 390

Quoted 5 lines

arch/arm/mach-imx/gpc.c +2 -0

+		.states = imx6q_arm_domain_states,
+		.state_count = ARRAY_SIZE(imx6q_arm_domain_states),
 	},
 };
 

How urgent is this patch? I have a series to rework the GPC driver almost ready and I could fold this change in directly if it's okay for this to sit through a review of the rework.

And I just noticed this depends on changes that are not already in v4.4-rc. Why is this sent as a single patch and not as part of the series adding multiple state support to genpd?

Regards,
Lucas

Signature · 2 lines

Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

From: Axel Haslam <[email protected]>

Now that all known users have been converted to use state latencies, we can remove the latency field in the generic_pm_domain structure.

Signed-off-by Axel Haslam <[email protected]>

---
 drivers/base/power/domain.c | 15 ---------------
 include/linux/pm_domain.h   |  2 --
 2 files changed, 17 deletions(-)

drivers/base/power/domain.c +0 -15

index 3fb4c93..c300293 100644
@@ -1248,21 +1248,6 @@ static int genpd_alloc_states_data(struct generic_pm_domain *genpd,
                genpd->states[i].param = st[i].param;
        }
 
-       /*
-        * Copy the latency values To keep compatibility with
-        * platforms that are not converted to use the multiple states.
-        * This will be removed once all platforms are converted to use
-        * multiple states. note that non converted platforms will use the
-        * default single off state.
-        */
-       if (genpd->power_on_latency_ns != 0)
-               genpd->states[0].power_on_latency_ns =
-                               genpd->power_on_latency_ns;
-
-       if (genpd->power_off_latency_ns != 0)
-               genpd->states[0].power_off_latency_ns =
-                               genpd->power_off_latency_ns;
-
        genpd->state_count = st_count;
 
        /* to save memory, Name allocation will happen if debug is enabled */

include/linux/pm_domain.h +0 -3

index 15df24c..108a4b3 100644
@@ -61,9 +61,7 @@ struct generic_pm_domain {
        unsigned int prepared_count;    /* Suspend counter of prepared devices */
        bool suspend_power_off; /* Power status before system suspend */
        int (*power_off)(struct generic_pm_domain *domain);
-       s64 power_off_latency_ns;
        int (*power_on)(struct generic_pm_domain *domain);
-       s64 power_on_latency_ns;
        struct gpd_dev_ops dev_ops;
        s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
        bool max_off_time_changed;
--

2.1.4

From: Marc Titinger <[email protected]>

This purpose of these debug seq-files is to help investigate generic power domain state transitions, based on device constraints. requires the "multiple states" patches from Axel Haslam.

also rename 'summary' from 'pm_genpd_summary'

sample output for 'states'
==========================

Domain State name Eval = 2200nter + Exit = Min_off_on (ns)
-----------------------------------------------------------------------

a53_pd               cluster-sleep-0      1500000+800000=2300000
a57_pd               d1-retention         1000000+800000=1800000
a57_pd               cluster-sleep-0      1500000+800000=2300000

sample output for 'timings'
===========================

Domain Devices, Timings in ns
Stop/Start Save/Restore, Effective
---------------------------------------------------- --- a53_pd

    /cpus/cpu at 100        1060  /660    1580  /1940  ,0 (cached stop)
    /cpus/cpu at 101        1060  /740    1520  /1600  ,0 (cached stop)
    /cpus/cpu at 102        880   /620    1380  /1780  ,0 (cached stop)
    /cpus/cpu at 103        1080  /640    1340  /1600  ,0 (cached stop)
a57_pd
    /cpus/cpu at 0          1160  /740    3280  /1800  ,0 (cached stop)
    /cpus/cpu at 1          780   /1400   1440  /2080  ,0 (cached stop)
    /D1                  600   /540    7140  /6420  ,2199460 (cached stop)

Signed-off-by Marc Titinger <[email protected]>

---
 drivers/base/power/domain.c | 115 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 107 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +0 -0

index c300293..9a0df09 100644
@@ -2169,21 +2169,120 @@ static const struct file_operations pm_genpd_summary_fops = {
        .release = single_release,
 };

drivers/base/power/domain.c +28 -0

+                       seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
+                                  genpd->name,
+                                  genpd->states[i].name,
+                                  genpd->states[i].power_on_latency_ns,
+                                  genpd->states[i].power_off_latency_ns,
+                                  genpd->states[i].power_off_latency_ns
+                                  + genpd->states[i].power_on_latency_ns);
+               }
+
+       }
+
+       seq_puts(s, "\n");
+
+       return 0;
+}
+
+static int pm_genpd_states_open(struct inode *inode, struct file *file)
+{
+       return single_open(file, pm_genpd_states_show, NULL);
+}
+
+static const struct file_operations pm_genpd_states_fops = {
+       .open = pm_genpd_states_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+};
+

drivers/base/power/domain.c +21 -0

+                                  td->effective_constraint_ns,
+                                  td->cached_stop_ok ? "(cached stop) " : "",
+                                  td->constraint_changed ? "(changed)" : "");
+               }
+               seq_puts(s, "\n");
+       }
+       return 0;
+}
+
+static int pm_genpd_timing_open(struct inode *inode, struct file *file)
+{
+       return single_open(file, pm_genpd_timing_show, NULL);
+}
+
+static const struct file_operations pm_genpd_timing_fops = {
+       .open = pm_genpd_timing_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+};
+

drivers/base/power/domain.c +13 -1

+               d = debugfs_create_file("summary", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_summary_fops);
+       if (d)
+               d = debugfs_create_file("states", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_states_fops);
+       if (d)
+               d = debugfs_create_file("timings", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_timing_fops);
+       if (d)
+               return 0;
 
-       d = debugfs_create_file("pm_genpd_summary", S_IRUGO,

On 17 November 2015 at 23:37, Lina Iyer <[email protected]> wrote:

Quoted 5 lines

From: Marc Titinger <[email protected]>

This purpose of these debug seq-files is to help investigate generic power domain state transitions, based on device constraints. requires the "multiple states" patches from Axel Haslam.

This last sentence doesn't belong in the change-log, please remove it.

Quoted 28 lines

also rename 'summary' from 'pm_genpd_summary'

sample output for 'states'
==========================

Domain State name Eval = 2200nter + Exit = Min_off_on (ns)
-----------------------------------------------------------------------

a53_pd               cluster-sleep-0      1500000+800000=2300000
a57_pd               d1-retention         1000000+800000=1800000
a57_pd               cluster-sleep-0      1500000+800000=2300000

sample output for 'timings'
===========================

Domain Devices, Timings in ns
Stop/Start Save/Restore, Effective
---------------------------------------------------- --- a53_pd

     /cpus/cpu at 100        1060  /660    1580  /1940  ,0 (cached stop)
     /cpus/cpu at 101        1060  /740    1520  /1600  ,0 (cached stop)
     /cpus/cpu at 102        880   /620    1380  /1780  ,0 (cached stop)
     /cpus/cpu at 103        1080  /640    1340  /1600  ,0 (cached stop)
a57_pd
     /cpus/cpu at 0          1160  /740    3280  /1800  ,0 (cached stop)
     /cpus/cpu at 1          780   /1400   1440  /2080  ,0 (cached stop)
     /D1                  600   /540    7140  /6420  ,2199460 (cached stop)

Quoted 1 line

Signed-off-by Marc Titinger <[email protected]>

A general comment. Static functions in genpd shall start with one of the following prefix.

genpd_*
_genpd_*
__genpd_*

Please change accordingly.

Quoted 12 lines

---
  drivers/base/power/domain.c | 115 +++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 107 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +0 -0

index c300293..9a0df09 100644
@@ -2169,21 +2169,120 @@ static const struct file_operations pm_genpd_summary_fops = {
         .release = single_release,
  };

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2180

Quoted 10 lines

drivers/base/power/domain.c +9 -0

 
+static int pm_genpd_states_show(struct seq_file *s, void *data)
+{
+       struct generic_pm_domain *genpd;
+
+       seq_puts(s,
+                "\n  Domain             State name        Enter + Exit = Min_off_on (ns)\n");
+       seq_puts(s,
+                "-----------------------------------------------------------------------\n");
+

You must hold the gpd_list_lock while traversing the gpd list.

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2185

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+
+               int i;
+
+               for (i = 0; i < genpd->state_count; i++) {

To be sure you have valid data, you should hold the genpd lock here.

Quoted 28 lines

drivers/base/power/domain.c +28 -0

+                       seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
+                                  genpd->name,
+                                  genpd->states[i].name,
+                                  genpd->states[i].power_on_latency_ns,
+                                  genpd->states[i].power_off_latency_ns,
+                                  genpd->states[i].power_off_latency_ns
+                                  + genpd->states[i].power_on_latency_ns);
+               }
+
+       }
+
+       seq_puts(s, "\n");
+
+       return 0;
+}
+
+static int pm_genpd_states_open(struct inode *inode, struct file *file)
+{
+       return single_open(file, pm_genpd_states_show, NULL);
+}
+
+static const struct file_operations pm_genpd_states_fops = {
+       .open = pm_genpd_states_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+};
+

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2223

Quoted 10 lines

drivers/base/power/domain.c +10 -0

+static int pm_genpd_timing_show(struct seq_file *s, void *data)
+{
+       struct generic_pm_domain *genpd;
+
+       seq_puts(s, "\n    Domain Devices, Timings in ns\n");
+       seq_puts(s,
+                "                       Stop/Start Save/Restore, Effective\n");
+       seq_puts(s,
+                "----------------------------------------------------  ---\n");
+

You must hold the gpd_list_lock while traversing the gpd list.

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2228

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+               struct pm_domain_data *pm_data;
+
+               seq_printf(s, "%-30s", genpd->name);
+

You must hold the genpd lock while traversing the device list.

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2240

Quoted 12 lines

drivers/base/power/domain.c +12 -0

+               list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
+                       struct gpd_timing_data *td = &to_gpd_data(pm_data)->td;
+
+                       if (!pm_data->dev->of_node)
+                               continue;
+
+                       seq_printf(s,
+                                  "\n    %-20s %-6lld/%-6lld %-6lld/%-6lld,%lld %s%s",
+                                  pm_data->dev->of_node->full_name,
+                                  td->stop_latency_ns, td->start_latency_ns,
+                                  td->save_state_latency_ns,
+                                  td->restore_state_latency_ns,

This needs a re-base as these values have been merged.

Quoted 21 lines

drivers/base/power/domain.c +21 -0

+                                  td->effective_constraint_ns,
+                                  td->cached_stop_ok ? "(cached stop) " : "",
+                                  td->constraint_changed ? "(changed)" : "");
+               }
+               seq_puts(s, "\n");
+       }
+       return 0;
+}
+
+static int pm_genpd_timing_open(struct inode *inode, struct file *file)
+{
+       return single_open(file, pm_genpd_timing_show, NULL);
+}
+
+static const struct file_operations pm_genpd_timing_fops = {
+       .open = pm_genpd_timing_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+};
+

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2268

Quoted 10 lines

drivers/base/power/domain.c +2 -3

  static int __init pm_genpd_debug_init(void)
  {
-       struct dentry *d;
+       struct dentry *d = NULL;
 
         pm_genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL);
 
-       if (!pm_genpd_debugfs_dir)
-               return -ENOMEM;
+       if (pm_genpd_debugfs_dir)

In case when CONFIG_DEBUG_FS is unset, this doesn't work very well, as pm_genpd_debugfs_dir will contain an error code.

Since you are anyway make quite big change to the debugfs support for genpd, perhaps you can try to fix that up as well!?

Quoted 16 lines

drivers/base/power/domain.c +13 -1

+               d = debugfs_create_file("summary", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_summary_fops);
+       if (d)
+               d = debugfs_create_file("states", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_states_fops);
+       if (d)
+               d = debugfs_create_file("timings", S_IRUGO,
+                               pm_genpd_debugfs_dir, NULL,
+                               &pm_genpd_timing_fops);
+       if (d)
+               return 0;
 
-       d = debugfs_create_file("pm_genpd_summary", S_IRUGO,

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2288

Quoted 10 lines

drivers/base/power/domain.c +2 -4

-                       pm_genpd_debugfs_dir, NULL, &pm_genpd_summary_fops);
-       if (!d)
-               return -ENOMEM;
+       debugfs_remove_recursive(pm_genpd_debugfs_dir /*can be null*/);
 
-       return 0;
+       return -ENOMEM;
  }
  late_initcall(pm_genpd_debug_init);
 

Kind regards
Uffe

Quoted 8 lines

A general comment. Static functions in genpd shall start with one of the following prefix.

genpd_*
_genpd_*
__genpd_*

Quoted 1 line

Please change accordingly.

Many static routines were already prefixed like the exported functions with "pm_", shall I make a separate patch for this renaming ?

Quoted 32 lines

Quoted 18 lines

---
  drivers/base/power/domain.c | 115
+++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 107 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +1 -0

index c300293..9a0df09 100644
@@ -2169,21 +2169,120 @@ static const struct file_operations
 pm_genpd_summary_fops = {
         .release = single_release,
  };
 
+static int pm_genpd_states_show(struct seq_file *s, void *data)

drivers/base/power/domain.c static const struct file_operations · line 2183

Quoted 10 lines

drivers/base/power/domain.c +8 -0

+{
+       struct generic_pm_domain *genpd;
+
+       seq_puts(s,
+                "\n  Domain             State name        Enter + Exit =
 Min_off_on (ns)\n");
+       seq_puts(s,
+
 "-----------------------------------------------------------------------\n");
+

You must hold the gpd_list_lock while traversing the gpd list.

Quoted 9 lines

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2185

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+
+               int i;
+
+               for (i = 0; i < genpd->state_count; i++) {

To be sure you have valid data, you should hold the genpd lock here.

At some point while testing with calling suspend from the power_off handler, the cpu would go to sleep with the lock held, hence using this seq-file would not work.

while I agree, I think it is not super likely that a domain/child/devices/states are added or removed at this point (the DT is parsed already), would using list_for_each_entry_safe be safe enough ?

Quoted 11 lines

Quoted 11 lines

drivers/base/power/domain.c +8 -0

+                       seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
+                                  genpd->name,
+                                  genpd->states[i].name,
+                                  genpd->states[i].power_on_latency_ns,
+                                  genpd->states[i].power_off_latency_ns,
+                                  genpd->states[i].power_off_latency_ns
+                                  +
 genpd->states[i].power_on_latency_ns);
+               }

Quoted 8 lines

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2201

Quoted 8 lines

drivers/base/power/domain.c +8 -0

+
+       }
+
+       seq_puts(s, "\n");
+
+       return 0;
+}
+

[...]

Quoted 8 lines

Quoted 8 lines

A general comment. Static functions in genpd shall start with one of the following prefix.

genpd_*
_genpd_*
__genpd_*

Quoted 5 lines

Quoted 1 line

Please change accordingly.

Many static routines were already prefixed like the exported functions with "pm_", shall I make a separate patch for this renaming ?

My point is that I don't want it to becomes worse.

If you follow the above rule for new changes, I am happy.

Whether you want to send a separate patch fixing the other existing name to be consistent with above rule, I would also be happy. :-)

Quoted 43 lines

Quoted 42 lines

Quoted 19 lines

---
   drivers/base/power/domain.c | 115
+++++++++++++++++++++++++++++++++++++++++---
   1 file changed, 107 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +1 -0

index c300293..9a0df09 100644
@@ -2169,21 +2169,120 @@ static const struct file_operations
 pm_genpd_summary_fops = {
          .release = single_release,
   };
 
+static int pm_genpd_states_show(struct seq_file *s, void *data)

drivers/base/power/domain.c static const struct file_operations · line 2183

Quoted 10 lines

drivers/base/power/domain.c +8 -0

+{
+       struct generic_pm_domain *genpd;
+
+       seq_puts(s,
+                "\n  Domain             State name        Enter + Exit =
 Min_off_on (ns)\n");
+       seq_puts(s,
+
 "-----------------------------------------------------------------------\n");
+

You must hold the gpd_list_lock while traversing the gpd list.

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2185

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+
+               int i;
+
+               for (i = 0; i < genpd->state_count; i++) {

To be sure you have valid data, you should hold the genpd lock here.

Quoted 3 lines

At some point while testing with calling suspend from the power_off handler, the cpu would go to sleep with the lock held, hence using this seq-file would not work.

That seems like a bad behaviour during suspend. Why does it hold the lock?

On the other it shouldn't matter as userspace can't access the debugfs nodes, since its frozen at those times, right!?

Quoted 5 lines

while I agree, I think it is not super likely that a domain/child/devices/states are added or removed at this point (the DT is parsed already), would using list_for_each_entry_safe be safe enough ?

No it's not.

The gpd_list is protected with the gdp_list_lock, which is needed because new genpds can be added at any point.

You also need the genpd lock here, as otherwise you may print the latency-values in the middle of when these are being updated.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/base/power/domain.c +8 -0

+                       seq_printf(s, "%-20s %-20s %lld+%lld=%lld\n",
+                                  genpd->name,
+                                  genpd->states[i].name,
+                                  genpd->states[i].power_on_latency_ns,
+                                  genpd->states[i].power_off_latency_ns,
+                                  genpd->states[i].power_off_latency_ns
+                                  +
 genpd->states[i].power_on_latency_ns);
+               }

Quoted 8 lines

Quoted 8 lines

drivers/base/power/domain.c static const struct file_operations pm_genpd_summary_fops = { · line 2201

Quoted 8 lines

drivers/base/power/domain.c +8 -0

+
+       }
+
+       seq_puts(s, "\n");
+
+       return 0;
+}
+

[...]

Kind regards
Uffe

Domains that have expensive suspend resume operations, may require a certain amount of time be spent in an idle state to reap the power saving benefit. Such domains, may provide the residency requirement for a domain state.

Read the residency for a domain state from the DT. A domain governor may use this information in determining the state for the domain.

Cc Axel Haslam <[email protected]>

Cc Marc Titinger <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 Documentation/devicetree/bindings/power/power_domain.txt | 7 +++++++