Instead of using bool to restore suspended devices initially, use flags
like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
The first two flags will be similar to the existing true/false functionality.
The third flag may be used to force restore of suspended devices
whenever their associated power domain is turned on.
Currently, PD power off function powers off all the associated unused
devices. The functionality added in this patch is similar to it.
This feature may be used for those devices which are always in ON state
if the PD associated with them is ON but may need local runtime resume
and suspend during PD On/Off. These devices (like clock) may not implement
complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
subsystems interaction behaviour or any other reason.
The model works like,
DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
/ pm_runtime_put. Its local runtime_suspend/resume is invoked via
/ GPD_DEV_RESTORE_FORCE option)
/
PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
\ pm_runtime_put. This in turn invokes PD On/Off)
\
DEV3 (Similar to DEV1)
Signed-off-by: Amit Daniel Kachhap <[email protected]>
---
This work is continuation of earlier work,
In V2: Completely removed notfiers and add support for huge clock list to be
suspended and restored. There was some issue in this as some clocks are
not exposed and are just initialised by bootloaders. This approach
required all of them to be exposed which is cumbersome. Also some
clock API's set_parent are disabling the original parent clocks
which is not required.
Link (https://lkml.org/lkml/2014/11/24/290)
In V1: Implemented PM Domain notfier such as PD_ON_PRE, PD_ON_POST
PD_OFF_PRE and PD_OFF_POST. This was not supported and other
options suggested. link (http://www.spinics.net/lists/linux-samsung-soc/msg38442.html)
This work may also assist exynos iommu pm runtime posted earlier by Marek
http://lists.linaro.org/pipermail/linaro-mm-sig/2014-August/004099.html
drivers/base/power/domain.c | 40 ++++++++++++++++++++++++++++++++++++----
include/linux/pm_domain.h | 15 +++++++++++++--
2 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 6a103a3..d955a05 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -49,6 +49,7 @@
static LIST_HEAD(gpd_list);
static DEFINE_MUTEX(gpd_list_lock);
+static void __pm_genpd_restore_devices(struct generic_pm_domain *genpd);
static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
{
@@ -176,6 +177,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
genpd->name, "on", elapsed_ns);
+ __pm_genpd_restore_devices(genpd);
+
return ret;
}
@@ -453,6 +456,24 @@ static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
}
/**
+ * __pm_genpd_restore_devices- Restore the pre-suspend state of all device
+ * according to the restore flag.
+ * @genpd: PM domain the device belongs to.
+ */
+static void __pm_genpd_restore_devices(struct generic_pm_domain *genpd)
+{
+ struct pm_domain_data *pdd;
+ struct generic_pm_domain_data *gpd_data;
+
+ /* Force restore the devices according to the restore flag */
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ gpd_data = to_gpd_data(pdd);
+ if (gpd_data->force_restore == true)
+ __pm_genpd_restore_device(pdd, genpd);
+ }
+}
+
+/**
* genpd_abort_poweroff - Check if a PM domain power off should be aborted.
* @genpd: PM domain to check.
*
@@ -1469,6 +1490,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
gpd_data->base.dev = dev;
list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
gpd_data->need_restore = -1;
+ gpd_data->force_restore = false;
gpd_data->td.constraint_changed = true;
gpd_data->td.effective_constraint_ns = -1;
mutex_unlock(&gpd_data->lock);
@@ -1561,18 +1583,28 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
/**
* pm_genpd_dev_need_restore - Set/unset the device's "need restore" flag.
* @dev: Device to set/unset the flag for.
- * @val: The new value of the device's "need restore" flag.
+ * @val: This can have values GPD_DEV_SUSPEND_INIT or GPD_DEV_RESTORE_INIT
+ * together with GPD_DEV_RESTORE_FORCE.
*/
-void pm_genpd_dev_need_restore(struct device *dev, bool val)
+void pm_genpd_dev_need_restore(struct device *dev, unsigned int val)
{
struct pm_subsys_data *psd;
+ struct generic_pm_domain_data *pdd;
unsigned long flags;
spin_lock_irqsave(&dev->power.lock, flags);
psd = dev_to_psd(dev);
- if (psd && psd->domain_data)
- to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
+ if (psd && psd->domain_data) {
+ pdd = to_gpd_data(psd->domain_data);
+ if (val & GPD_DEV_SUSPEND_INIT)
+ pdd->need_restore = 0;
+ else if (val & GPD_DEV_RESTORE_INIT)
+ pdd->need_restore = 1;
+
+ if (val & GPD_DEV_RESTORE_FORCE)
+ pdd->force_restore = true;
+ }
spin_unlock_irqrestore(&dev->power.lock, flags);
}
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 6cd20d5..8aa74ee 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -20,6 +20,15 @@
/* Defines used for the flags field in the struct generic_pm_domain */
#define GENPD_FLAG_PM_CLK (1U << 0) /* PM domain uses PM clk */
+/* Defines used for the types of restore */
+#define GPD_DEV_SUSPEND_INIT 0x1 /* Initiate the device runtime as
+ suspend/resume/suspend... */
+#define GPD_DEV_RESTORE_INIT 0x10 /* Initiate the device runtime as
+ resume/suspend/suspend... */
+#define GPD_DEV_RESTORE_FORCE 0x100 /* Force these devices to always do
+ resume in PD power on. Initial
+ behaviour can be one of the above */
+
enum gpd_status {
GPD_STATE_ACTIVE = 0, /* PM domain is active */
GPD_STATE_WAIT_MASTER, /* PM domain's master is being waited for */
@@ -116,6 +125,7 @@ struct generic_pm_domain_data {
struct mutex lock;
unsigned int refcount;
int need_restore;
+ bool force_restore;
};
#ifdef CONFIG_PM_GENERIC_DOMAINS
@@ -140,7 +150,7 @@ extern int __pm_genpd_name_add_device(const char *domain_name,
extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
struct device *dev);
-extern void pm_genpd_dev_need_restore(struct device *dev, bool val);
+extern void pm_genpd_dev_need_restore(struct device *dev, unsigned int val);
extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_subdomain);
extern int pm_genpd_add_subdomain_names(const char *master_name,
@@ -187,7 +197,8 @@ static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
{
return -ENOSYS;
}
-static inline void pm_genpd_dev_need_restore(struct device *dev, bool val) {}
+static inline void pm_genpd_dev_need_restore(struct device *dev,
+ unsigned int val) {}
static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_sd)
{
--
2.2.0
This patch adds PM runtime support for clocks associated with Power
Domain. The PM runtime suspend/resume handlers will be called when the
power domain associated with it, is turned on/off.
The registration of clocks happen in early initailisation. The probe
is later called to register the clock device with the power domain.
Signed-off-by: Amit Daniel Kachhap <[email protected]>
---
drivers/clk/samsung/clk.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/clk/samsung/clk.h | 11 ++++++
2 files changed, 106 insertions(+)
diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index dd1f7c9..c517aa8 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -11,6 +11,10 @@
* clock framework for Samsung platforms.
*/
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
#include <linux/syscore_ops.h>
#include "clk.h"
@@ -370,6 +374,93 @@ static void samsung_clk_sleep_init(void __iomem *reg_base,
unsigned long nr_rdump) {}
#endif
+static int samsung_cmu_runtime_suspend(struct device *dev)
+{
+ struct samsung_clock_pd_reg_cache *reg_cache;
+
+ reg_cache = dev_get_drvdata(dev);
+ samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
+ reg_cache->rd_num);
+ return 0;
+}
+
+static int samsung_cmu_runtime_resume(struct device *dev)
+{
+ struct samsung_clock_pd_reg_cache *reg_cache;
+
+ reg_cache = dev_get_drvdata(dev);
+ samsung_clk_restore(reg_cache->reg_base, reg_cache->rdump,
+ reg_cache->rd_num);
+ return 0;
+}
+
+#define MAX_CMU_DEVICE_MATCH 50
+static int samsung_cmu_count;
+static struct of_device_id samsung_cmu_match[MAX_CMU_DEVICE_MATCH];
+MODULE_DEVICE_TABLE(of, samsung_cmu_match);
+
+static void samsung_clk_pd_init(struct device_node *np, void __iomem *reg_base,
+ struct samsung_cmu_info *cmu)
+{
+ struct samsung_clock_pd_reg_cache *pd_reg_cache;
+ const char *name;
+
+ if (samsung_cmu_count == MAX_CMU_DEVICE_MATCH)
+ panic("Maximum clock device limit reached.\n");
+
+ if (of_property_read_string_index(np, "compatible", 0, &name))
+ panic("Invalid DT node.\n");
+
+ pd_reg_cache = kzalloc(sizeof(struct samsung_clock_pd_reg_cache),
+ GFP_KERNEL);
+ if (!pd_reg_cache)
+ panic("Could not allocate register reg_cache.\n");
+
+ pd_reg_cache->rdump = samsung_clk_alloc_reg_dump(cmu->pd_clk_regs,
+ cmu->nr_pd_clk_regs);
+ if (!pd_reg_cache->rdump)
+ panic("Could not allocate register dump storage.\n");
+
+ pd_reg_cache->reg_base = reg_base;
+ pd_reg_cache->rd_num = cmu->nr_pd_clk_regs;
+
+ /* Fill up the compatible string and data */
+ samsung_cmu_match[samsung_cmu_count].data = pd_reg_cache;
+ strcpy(samsung_cmu_match[samsung_cmu_count].compatible, name);
+ samsung_cmu_count++;
+}
+
+static int __init samsung_cmu_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct of_device_id *match;
+
+ /* get the platform data */
+ match = (struct of_device_id *)of_match_node(samsung_cmu_match,
+ pdev->dev.of_node);
+ if (!match)
+ return 0;
+ platform_set_drvdata(pdev, (void *)match->data);
+ pm_runtime_enable(dev);
+ pm_genpd_dev_need_restore(dev,
+ GPD_DEV_RESTORE_FORCE | GPD_DEV_SUSPEND_INIT);
+ return 0;
+}
+
+static const struct dev_pm_ops samsung_cmu_pm_ops = {
+ SET_RUNTIME_PM_OPS(samsung_cmu_runtime_suspend,
+ samsung_cmu_runtime_resume, NULL)
+};
+
+static struct platform_driver samsung_cmu_driver = {
+ .driver = {
+ .name = "exynos-clk",
+ .of_match_table = samsung_cmu_match,
+ .pm = &samsung_cmu_pm_ops,
+ },
+ .probe = samsung_cmu_probe,
+};
+
/*
* Common function which registers plls, muxes, dividers and gates
* for each CMU. It also add CMU register list to register cache.
@@ -409,5 +500,9 @@ void __init samsung_cmu_register_one(struct device_node *np,
samsung_clk_sleep_init(reg_base, cmu->clk_regs,
cmu->nr_clk_regs);
+ if (cmu->pd_clk_regs)
+ samsung_clk_pd_init(np, reg_base, cmu);
+
samsung_clk_of_add_provider(np, ctx);
}
+module_platform_driver(samsung_cmu_driver);
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index 3f471e9..c184ec1 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -331,6 +331,12 @@ struct samsung_clock_reg_cache {
unsigned int rd_num;
};
+struct samsung_clock_pd_reg_cache {
+ void __iomem *reg_base;
+ struct samsung_clk_reg_dump *rdump;
+ unsigned int rd_num;
+};
+
struct samsung_cmu_info {
/* list of pll clocks and respective count */
struct samsung_pll_clock *pll_clks;
@@ -356,6 +362,11 @@ struct samsung_cmu_info {
/* list and number of clocks registers */
unsigned long *clk_regs;
unsigned int nr_clk_regs;
+
+ /* list and number of clocks to be saved/restored during
+ * power domain shutdown */
+ unsigned long *pd_clk_regs;
+ unsigned int nr_pd_clk_regs;
};
extern struct samsung_clk_provider *__init samsung_clk_init(
--
2.2.0
Hello,
On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
> Instead of using bool to restore suspended devices initially, use flags
> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
> The first two flags will be similar to the existing true/false functionality.
> The third flag may be used to force restore of suspended devices
> whenever their associated power domain is turned on.
>
> Currently, PD power off function powers off all the associated unused
> devices. The functionality added in this patch is similar to it.
>
> This feature may be used for those devices which are always in ON state
> if the PD associated with them is ON but may need local runtime resume
> and suspend during PD On/Off. These devices (like clock) may not implement
> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
> subsystems interaction behaviour or any other reason.
>
> The model works like,
> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
> / GPD_DEV_RESTORE_FORCE option)
> /
> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
> \ pm_runtime_put. This in turn invokes PD On/Off)
> \
> DEV3 (Similar to DEV1)
>
> Signed-off-by: Amit Daniel Kachhap <[email protected]>
The idea of adding new gen_pd flag and reusing runtime pm calls intead
of additional
notifiers looks promising, but I have some doubts. I don't see any
guarantee that
devices with GPD_DEV_RESTORE_FORCE flag will be suspended after all
"normal" devices
and restored before them. Without such assumption it will be hard to use
this approach
for iommu related activities, because device might need to use (in its
suspend/resume
callbacks) the functionality provided by the other device with
GPD_DEV_RESTORE_FORCE
flag. Maybe some additional flags like suspend/resume priority (or more
flags) will
solve somehow this dependency.
> ---
> This work is continuation of earlier work,
>
> In V2: Completely removed notfiers and add support for huge clock list to be
> suspended and restored. There was some issue in this as some clocks are
> not exposed and are just initialised by bootloaders. This approach
> required all of them to be exposed which is cumbersome. Also some
> clock API's set_parent are disabling the original parent clocks
> which is not required.
> Link (https://lkml.org/lkml/2014/11/24/290)
>
> In V1: Implemented PM Domain notfier such as PD_ON_PRE, PD_ON_POST
> PD_OFF_PRE and PD_OFF_POST. This was not supported and other
> options suggested. link (http://www.spinics.net/lists/linux-samsung-soc/msg38442.html)
>
> This work may also assist exynos iommu pm runtime posted earlier by Marek
> http://lists.linaro.org/pipermail/linaro-mm-sig/2014-August/004099.html
>
> drivers/base/power/domain.c | 40 ++++++++++++++++++++++++++++++++++++----
> include/linux/pm_domain.h | 15 +++++++++++++--
> 2 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 6a103a3..d955a05 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -49,6 +49,7 @@
>
> static LIST_HEAD(gpd_list);
> static DEFINE_MUTEX(gpd_list_lock);
> +static void __pm_genpd_restore_devices(struct generic_pm_domain *genpd);
>
> static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> {
> @@ -176,6 +177,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
> pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
> genpd->name, "on", elapsed_ns);
>
> + __pm_genpd_restore_devices(genpd);
> +
> return ret;
> }
>
> @@ -453,6 +456,24 @@ static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
> }
>
> /**
> + * __pm_genpd_restore_devices- Restore the pre-suspend state of all device
> + * according to the restore flag.
> + * @genpd: PM domain the device belongs to.
> + */
> +static void __pm_genpd_restore_devices(struct generic_pm_domain *genpd)
> +{
> + struct pm_domain_data *pdd;
> + struct generic_pm_domain_data *gpd_data;
> +
> + /* Force restore the devices according to the restore flag */
> + list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> + gpd_data = to_gpd_data(pdd);
> + if (gpd_data->force_restore == true)
> + __pm_genpd_restore_device(pdd, genpd);
> + }
> +}
> +
> +/**
> * genpd_abort_poweroff - Check if a PM domain power off should be aborted.
> * @genpd: PM domain to check.
> *
> @@ -1469,6 +1490,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> gpd_data->base.dev = dev;
> list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> gpd_data->need_restore = -1;
> + gpd_data->force_restore = false;
> gpd_data->td.constraint_changed = true;
> gpd_data->td.effective_constraint_ns = -1;
> mutex_unlock(&gpd_data->lock);
> @@ -1561,18 +1583,28 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> /**
> * pm_genpd_dev_need_restore - Set/unset the device's "need restore" flag.
> * @dev: Device to set/unset the flag for.
> - * @val: The new value of the device's "need restore" flag.
> + * @val: This can have values GPD_DEV_SUSPEND_INIT or GPD_DEV_RESTORE_INIT
> + * together with GPD_DEV_RESTORE_FORCE.
> */
> -void pm_genpd_dev_need_restore(struct device *dev, bool val)
> +void pm_genpd_dev_need_restore(struct device *dev, unsigned int val)
> {
> struct pm_subsys_data *psd;
> + struct generic_pm_domain_data *pdd;
> unsigned long flags;
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> psd = dev_to_psd(dev);
> - if (psd && psd->domain_data)
> - to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
> + if (psd && psd->domain_data) {
> + pdd = to_gpd_data(psd->domain_data);
> + if (val & GPD_DEV_SUSPEND_INIT)
> + pdd->need_restore = 0;
> + else if (val & GPD_DEV_RESTORE_INIT)
> + pdd->need_restore = 1;
> +
> + if (val & GPD_DEV_RESTORE_FORCE)
> + pdd->force_restore = true;
> + }
>
> spin_unlock_irqrestore(&dev->power.lock, flags);
> }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 6cd20d5..8aa74ee 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -20,6 +20,15 @@
> /* Defines used for the flags field in the struct generic_pm_domain */
> #define GENPD_FLAG_PM_CLK (1U << 0) /* PM domain uses PM clk */
>
> +/* Defines used for the types of restore */
> +#define GPD_DEV_SUSPEND_INIT 0x1 /* Initiate the device runtime as
> + suspend/resume/suspend... */
> +#define GPD_DEV_RESTORE_INIT 0x10 /* Initiate the device runtime as
> + resume/suspend/suspend... */
> +#define GPD_DEV_RESTORE_FORCE 0x100 /* Force these devices to always do
> + resume in PD power on. Initial
> + behaviour can be one of the above */
> +
> enum gpd_status {
> GPD_STATE_ACTIVE = 0, /* PM domain is active */
> GPD_STATE_WAIT_MASTER, /* PM domain's master is being waited for */
> @@ -116,6 +125,7 @@ struct generic_pm_domain_data {
> struct mutex lock;
> unsigned int refcount;
> int need_restore;
> + bool force_restore;
> };
>
> #ifdef CONFIG_PM_GENERIC_DOMAINS
> @@ -140,7 +150,7 @@ extern int __pm_genpd_name_add_device(const char *domain_name,
>
> extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> struct device *dev);
> -extern void pm_genpd_dev_need_restore(struct device *dev, bool val);
> +extern void pm_genpd_dev_need_restore(struct device *dev, unsigned int val);
> extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> struct generic_pm_domain *new_subdomain);
> extern int pm_genpd_add_subdomain_names(const char *master_name,
> @@ -187,7 +197,8 @@ static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> {
> return -ENOSYS;
> }
> -static inline void pm_genpd_dev_need_restore(struct device *dev, bool val) {}
> +static inline void pm_genpd_dev_need_restore(struct device *dev,
> + unsigned int val) {}
> static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> struct generic_pm_domain *new_sd)
> {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Marek Szyprowski <[email protected]> writes:
> Hello,
>
> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
>> Instead of using bool to restore suspended devices initially, use flags
>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
>> The first two flags will be similar to the existing true/false functionality.
>> The third flag may be used to force restore of suspended devices
>> whenever their associated power domain is turned on.
>>
>> Currently, PD power off function powers off all the associated unused
>> devices. The functionality added in this patch is similar to it.
>>
>> This feature may be used for those devices which are always in ON state
>> if the PD associated with them is ON but may need local runtime resume
>> and suspend during PD On/Off. These devices (like clock) may not implement
>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
>> subsystems interaction behaviour or any other reason.
>>
>> The model works like,
>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
>> / GPD_DEV_RESTORE_FORCE option)
>> /
>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
>> \ pm_runtime_put. This in turn invokes PD On/Off)
>> \
>> DEV3 (Similar to DEV1)
>>
>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>
> The idea of adding new gen_pd flag and reusing runtime pm calls intead
> of additional notifiers looks promising, but I have some doubts.
I agree, this is better than notifiers, but I have some doubts too.
> don't see any guarantee that devices with GPD_DEV_RESTORE_FORCE flag
> will be suspended after all "normal" devices and restored before
> them. Without such assumption it will be hard to use this approach for
> iommu related activities, because device might need to use (in its
> suspend/resume callbacks) the functionality provided by the other
> device with GPD_DEV_RESTORE_FORCE flag. Maybe some additional flags
> like suspend/resume priority (or more flags) will solve somehow this
> dependency.
At a deeper level, the problem with this approach is that this is more
generically a runtime PM dependency problem, not a genpd problem. For
example, what happens when the same kind of dependency exists on a
platform using a custom PM domain instead of genpd (like ACPI.) ?
What's needed to solve this problem is a generalized way to have runtime
PM dependencies between devices. Runtime PM already automatically
handles parent devices as one type of dependent device (e.g. a parent
device needs to be runtime PM resumed before its child.) So what's
needed is a generic way to other PM dependencies with the runtime PM
core (not the genpd core.)
If runtime PM handles the dependencies corretcly, then genpd (and any
other PM domain) will get them "for free".
Kevin
On Tue, Dec 16, 2014 at 4:40 PM, Marek Szyprowski
<[email protected]> wrote:
> Hello,
>
> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
>>
>> Instead of using bool to restore suspended devices initially, use flags
>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
>> The first two flags will be similar to the existing true/false
>> functionality.
>> The third flag may be used to force restore of suspended devices
>> whenever their associated power domain is turned on.
>>
>> Currently, PD power off function powers off all the associated unused
>> devices. The functionality added in this patch is similar to it.
>>
>> This feature may be used for those devices which are always in ON state
>> if the PD associated with them is ON but may need local runtime resume
>> and suspend during PD On/Off. These devices (like clock) may not implement
>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
>> subsystems interaction behaviour or any other reason.
>>
>> The model works like,
>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
>> / GPD_DEV_RESTORE_FORCE option)
>> /
>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
>> \ pm_runtime_put. This in turn invokes PD On/Off)
>> \
>> DEV3 (Similar to DEV1)
>>
>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>
>
> The idea of adding new gen_pd flag and reusing runtime pm calls intead of
> additional
> notifiers looks promising, but I have some doubts. I don't see any guarantee
> that
> devices with GPD_DEV_RESTORE_FORCE flag will be suspended after all "normal"
> devices
> and restored before them. Without such assumption it will be hard to use
> this approach
> for iommu related activities, because device might need to use (in its
> suspend/resume
> callbacks) the functionality provided by the other device with
> GPD_DEV_RESTORE_FORCE
> flag. Maybe some additional flags like suspend/resume priority (or more
> flags) will
> solve somehow this dependency.
Hi Marek,
Thanks for pointing this issue out. I agree that there is no priority
in suspending devices with this flag. But this works well in syncing
with power domain on/off as the devices of these types have only
dependency with power domain. BTW I tested this implementation with
your first version of IOMMU save/restore patches. Although i have not
posted those but they work fine. I can post those after cleaning them
up.
Here, the ordering between various devices is taken care as they are
probed at different point of time(clock then IOMMU). so the suspend
happens in reverse probe order and resume happens in probe order. I
will check more on this and get back.
Thanks,
Amit Daniel
>
>
>> ---
>> This work is continuation of earlier work,
>>
>> In V2: Completely removed notfiers and add support for huge clock list to
>> be
>> suspended and restored. There was some issue in this as some
>> clocks are
>> not exposed and are just initialised by bootloaders. This approach
>> required all of them to be exposed which is cumbersome. Also some
>> clock API's set_parent are disabling the original parent clocks
>> which is not required.
>> Link (https://lkml.org/lkml/2014/11/24/290)
>>
>> In V1: Implemented PM Domain notfier such as PD_ON_PRE, PD_ON_POST
>> PD_OFF_PRE and PD_OFF_POST. This was not supported and other
>> options suggested. link
>> (http://www.spinics.net/lists/linux-samsung-soc/msg38442.html)
>>
>> This work may also assist exynos iommu pm runtime posted earlier by Marek
>> http://lists.linaro.org/pipermail/linaro-mm-sig/2014-August/004099.html
>>
>> drivers/base/power/domain.c | 40
>> ++++++++++++++++++++++++++++++++++++----
>> include/linux/pm_domain.h | 15 +++++++++++++--
>> 2 files changed, 49 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 6a103a3..d955a05 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -49,6 +49,7 @@
>> static LIST_HEAD(gpd_list);
>> static DEFINE_MUTEX(gpd_list_lock);
>> +static void __pm_genpd_restore_devices(struct generic_pm_domain *genpd);
>> static struct generic_pm_domain *pm_genpd_lookup_name(const char
>> *domain_name)
>> {
>> @@ -176,6 +177,8 @@ static int genpd_power_on(struct generic_pm_domain
>> *genpd)
>> pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
>> genpd->name, "on", elapsed_ns);
>> + __pm_genpd_restore_devices(genpd);
>> +
>> return ret;
>> }
>> @@ -453,6 +456,24 @@ static void __pm_genpd_restore_device(struct
>> pm_domain_data *pdd,
>> }
>> /**
>> + * __pm_genpd_restore_devices- Restore the pre-suspend state of all
>> device
>> + * according to the restore flag.
>> + * @genpd: PM domain the device belongs to.
>> + */
>> +static void __pm_genpd_restore_devices(struct generic_pm_domain *genpd)
>> +{
>> + struct pm_domain_data *pdd;
>> + struct generic_pm_domain_data *gpd_data;
>> +
>> + /* Force restore the devices according to the restore flag */
>> + list_for_each_entry(pdd, &genpd->dev_list, list_node) {
>> + gpd_data = to_gpd_data(pdd);
>> + if (gpd_data->force_restore == true)
>> + __pm_genpd_restore_device(pdd, genpd);
>> + }
>> +}
>> +
>> +/**
>> * genpd_abort_poweroff - Check if a PM domain power off should be
>> aborted.
>> * @genpd: PM domain to check.
>> *
>> @@ -1469,6 +1490,7 @@ int __pm_genpd_add_device(struct generic_pm_domain
>> *genpd, struct device *dev,
>> gpd_data->base.dev = dev;
>> list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>> gpd_data->need_restore = -1;
>> + gpd_data->force_restore = false;
>> gpd_data->td.constraint_changed = true;
>> gpd_data->td.effective_constraint_ns = -1;
>> mutex_unlock(&gpd_data->lock);
>> @@ -1561,18 +1583,28 @@ int pm_genpd_remove_device(struct
>> generic_pm_domain *genpd,
>> /**
>> * pm_genpd_dev_need_restore - Set/unset the device's "need restore"
>> flag.
>> * @dev: Device to set/unset the flag for.
>> - * @val: The new value of the device's "need restore" flag.
>> + * @val: This can have values GPD_DEV_SUSPEND_INIT or
>> GPD_DEV_RESTORE_INIT
>> + * together with GPD_DEV_RESTORE_FORCE.
>> */
>> -void pm_genpd_dev_need_restore(struct device *dev, bool val)
>> +void pm_genpd_dev_need_restore(struct device *dev, unsigned int val)
>> {
>> struct pm_subsys_data *psd;
>> + struct generic_pm_domain_data *pdd;
>> unsigned long flags;
>> spin_lock_irqsave(&dev->power.lock, flags);
>> psd = dev_to_psd(dev);
>> - if (psd && psd->domain_data)
>> - to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
>> + if (psd && psd->domain_data) {
>> + pdd = to_gpd_data(psd->domain_data);
>> + if (val & GPD_DEV_SUSPEND_INIT)
>> + pdd->need_restore = 0;
>> + else if (val & GPD_DEV_RESTORE_INIT)
>> + pdd->need_restore = 1;
>> +
>> + if (val & GPD_DEV_RESTORE_FORCE)
>> + pdd->force_restore = true;
>> + }
>> spin_unlock_irqrestore(&dev->power.lock, flags);
>> }
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 6cd20d5..8aa74ee 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -20,6 +20,15 @@
>> /* Defines used for the flags field in the struct generic_pm_domain */
>> #define GENPD_FLAG_PM_CLK (1U << 0) /* PM domain uses PM clk */
>> +/* Defines used for the types of restore */
>> +#define GPD_DEV_SUSPEND_INIT 0x1 /* Initiate the device
>> runtime as
>> + suspend/resume/suspend... */
>> +#define GPD_DEV_RESTORE_INIT 0x10 /* Initiate the device
>> runtime as
>> + resume/suspend/suspend... */
>> +#define GPD_DEV_RESTORE_FORCE 0x100 /* Force these devices to
>> always do
>> + resume in PD power on. Initial
>> + behaviour can be one of the
>> above */
>> +
>> enum gpd_status {
>> GPD_STATE_ACTIVE = 0, /* PM domain is active */
>> GPD_STATE_WAIT_MASTER, /* PM domain's master is being waited for
>> */
>> @@ -116,6 +125,7 @@ struct generic_pm_domain_data {
>> struct mutex lock;
>> unsigned int refcount;
>> int need_restore;
>> + bool force_restore;
>> };
>> #ifdef CONFIG_PM_GENERIC_DOMAINS
>> @@ -140,7 +150,7 @@ extern int __pm_genpd_name_add_device(const char
>> *domain_name,
>> extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>> struct device *dev);
>> -extern void pm_genpd_dev_need_restore(struct device *dev, bool val);
>> +extern void pm_genpd_dev_need_restore(struct device *dev, unsigned int
>> val);
>> extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>> struct generic_pm_domain
>> *new_subdomain);
>> extern int pm_genpd_add_subdomain_names(const char *master_name,
>> @@ -187,7 +197,8 @@ static inline int pm_genpd_remove_device(struct
>> generic_pm_domain *genpd,
>> {
>> return -ENOSYS;
>> }
>> -static inline void pm_genpd_dev_need_restore(struct device *dev, bool
>> val) {}
>> +static inline void pm_genpd_dev_need_restore(struct device *dev,
>> + unsigned int val) {}
>> static inline int pm_genpd_add_subdomain(struct generic_pm_domain
>> *genpd,
>> struct generic_pm_domain *new_sd)
>> {
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello,
On 2014-12-17 03:43, amit daniel kachhap wrote:
> On Tue, Dec 16, 2014 at 4:40 PM, Marek Szyprowski
> <[email protected]> wrote:
>> Hello,
>>
>> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
>>> Instead of using bool to restore suspended devices initially, use flags
>>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
>>> The first two flags will be similar to the existing true/false
>>> functionality.
>>> The third flag may be used to force restore of suspended devices
>>> whenever their associated power domain is turned on.
>>>
>>> Currently, PD power off function powers off all the associated unused
>>> devices. The functionality added in this patch is similar to it.
>>>
>>> This feature may be used for those devices which are always in ON state
>>> if the PD associated with them is ON but may need local runtime resume
>>> and suspend during PD On/Off. These devices (like clock) may not implement
>>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
>>> subsystems interaction behaviour or any other reason.
>>>
>>> The model works like,
>>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
>>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
>>> / GPD_DEV_RESTORE_FORCE option)
>>> /
>>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
>>> \ pm_runtime_put. This in turn invokes PD On/Off)
>>> \
>>> DEV3 (Similar to DEV1)
>>>
>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>>
>> The idea of adding new gen_pd flag and reusing runtime pm calls intead of
>> additional
>> notifiers looks promising, but I have some doubts. I don't see any guarantee
>> that
>> devices with GPD_DEV_RESTORE_FORCE flag will be suspended after all "normal"
>> devices
>> and restored before them. Without such assumption it will be hard to use
>> this approach
>> for iommu related activities, because device might need to use (in its
>> suspend/resume
>> callbacks) the functionality provided by the other device with
>> GPD_DEV_RESTORE_FORCE
>> flag. Maybe some additional flags like suspend/resume priority (or more
>> flags) will
>> solve somehow this dependency.
> Thanks for pointing this issue out. I agree that there is no priority
> in suspending devices with this flag. But this works well in syncing
> with power domain on/off as the devices of these types have only
> dependency with power domain. BTW I tested this implementation with
> your first version of IOMMU save/restore patches. Although i have not
> posted those but they work fine. I can post those after cleaning them
> up.
Right, they will work, but mainly because right now master devices don't
do any memory DMA operations in suspend/resume callbacks. However, to
propose
it as a generic solution, the priority and the order of operations
should be
somehow determined.
> Here, the ordering between various devices is taken care as they are
> probed at different point of time(clock then IOMMU). so the suspend
> happens in reverse probe order and resume happens in probe order. I
> will check more on this and get back.
Right now, it probably used the order of probing. Because iommu controllers
are probed earlier, so they are handled before the master devices.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Wed, Dec 17, 2014 at 3:40 AM, Kevin Hilman <[email protected]> wrote:
> Marek Szyprowski <[email protected]> writes:
>
>> Hello,
>>
>> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
>>> Instead of using bool to restore suspended devices initially, use flags
>>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
>>> The first two flags will be similar to the existing true/false functionality.
>>> The third flag may be used to force restore of suspended devices
>>> whenever their associated power domain is turned on.
>>>
>>> Currently, PD power off function powers off all the associated unused
>>> devices. The functionality added in this patch is similar to it.
>>>
>>> This feature may be used for those devices which are always in ON state
>>> if the PD associated with them is ON but may need local runtime resume
>>> and suspend during PD On/Off. These devices (like clock) may not implement
>>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
>>> subsystems interaction behaviour or any other reason.
>>>
>>> The model works like,
>>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
>>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
>>> / GPD_DEV_RESTORE_FORCE option)
>>> /
>>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
>>> \ pm_runtime_put. This in turn invokes PD On/Off)
>>> \
>>> DEV3 (Similar to DEV1)
>>>
>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>>
>> The idea of adding new gen_pd flag and reusing runtime pm calls intead
>> of additional notifiers looks promising, but I have some doubts.
>
> I agree, this is better than notifiers, but I have some doubts too.
Thanks,
>
>> don't see any guarantee that devices with GPD_DEV_RESTORE_FORCE flag
>> will be suspended after all "normal" devices and restored before
>> them. Without such assumption it will be hard to use this approach for
>> iommu related activities, because device might need to use (in its
>> suspend/resume callbacks) the functionality provided by the other
>> device with GPD_DEV_RESTORE_FORCE flag. Maybe some additional flags
>> like suspend/resume priority (or more flags) will solve somehow this
>> dependency.
>
> At a deeper level, the problem with this approach is that this is more
> generically a runtime PM dependency problem, not a genpd problem. For
> example, what happens when the same kind of dependency exists on a
> platform using a custom PM domain instead of genpd (like ACPI.) ?
This patch does not try to solve runtime PM dependencies between
devices. As an example, if there are three devices D1, D2, D3 in a
power domain. Device D3 would update the power domain state
requirement using runtime PM API but devices D1 and D2 do not want to
control the domain but just want to be notified when the power domain
state changes.
>
> What's needed to solve this problem is a generalized way to have runtime
> PM dependencies between devices. Runtime PM already automatically
> handles parent devices as one type of dependent device (e.g. a parent
> device needs to be runtime PM resumed before its child.) So what's
> needed is a generic way to other PM dependencies with the runtime PM
> core (not the genpd core.)
Considering the example above with three devices, device D1 and D2 are
passive components in this power domain. These devices only need to
know the state changes of the power domains but would not control the
power domain themselves nor put forth constraints in the power domain
state changes. So I did not clearly understand as to how this example
could be solved by introducing changes in runtime PM core.
Regards,
Amit Daniel
>
> If runtime PM handles the dependencies corretcly, then genpd (and any
> other PM domain) will get them "for free".
>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 17, 2014 at 6:38 PM, Marek Szyprowski
<[email protected]> wrote:
> Hello,
>
>
> On 2014-12-17 03:43, amit daniel kachhap wrote:
>>
>> On Tue, Dec 16, 2014 at 4:40 PM, Marek Szyprowski
>> <[email protected]> wrote:
>>>
>>> Hello,
>>>
>>> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
>>>>
>>>> Instead of using bool to restore suspended devices initially, use flags
>>>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and
>>>> GPD_DEV_RESTORE_FORCE.
>>>> The first two flags will be similar to the existing true/false
>>>> functionality.
>>>> The third flag may be used to force restore of suspended devices
>>>> whenever their associated power domain is turned on.
>>>>
>>>> Currently, PD power off function powers off all the associated unused
>>>> devices. The functionality added in this patch is similar to it.
>>>>
>>>> This feature may be used for those devices which are always in ON state
>>>> if the PD associated with them is ON but may need local runtime resume
>>>> and suspend during PD On/Off. These devices (like clock) may not
>>>> implement
>>>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
>>>> subsystems interaction behaviour or any other reason.
>>>>
>>>> The model works like,
>>>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
>>>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
>>>> / GPD_DEV_RESTORE_FORCE option)
>>>> /
>>>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
>>>> \ pm_runtime_put. This in turn invokes PD On/Off)
>>>> \
>>>> DEV3 (Similar to DEV1)
>>>>
>>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>>>
>>>
>>> The idea of adding new gen_pd flag and reusing runtime pm calls intead of
>>> additional
>>> notifiers looks promising, but I have some doubts. I don't see any
>>> guarantee
>>> that
>>> devices with GPD_DEV_RESTORE_FORCE flag will be suspended after all
>>> "normal"
>>> devices
>>> and restored before them. Without such assumption it will be hard to use
>>> this approach
>>> for iommu related activities, because device might need to use (in its
>>> suspend/resume
>>> callbacks) the functionality provided by the other device with
>>> GPD_DEV_RESTORE_FORCE
>>> flag. Maybe some additional flags like suspend/resume priority (or more
>>> flags) will
>>> solve somehow this dependency.
>>
>> Thanks for pointing this issue out. I agree that there is no priority
>> in suspending devices with this flag. But this works well in syncing
>> with power domain on/off as the devices of these types have only
>> dependency with power domain. BTW I tested this implementation with
>> your first version of IOMMU save/restore patches. Although i have not
>> posted those but they work fine. I can post those after cleaning them
>> up.
>
>
> Right, they will work, but mainly because right now master devices don't
> do any memory DMA operations in suspend/resume callbacks. However, to
> propose
> it as a generic solution, the priority and the order of operations should be
> somehow determined.
Hi,
I feel this behavior is analogous to system sleep where the devices
suspend/resume are called based on initialization types like early
init, pre core, post core , late init etc.
These types handle the priority and interdependency among devices. So,
In my opinion we can use the same for power domain also. if these
types are not sufficient enough than it may be considered for future
work.
Regards,
Amit Daniel
>
>> Here, the ordering between various devices is taken care as they are
>> probed at different point of time(clock then IOMMU). so the suspend
>> happens in reverse probe order and resume happens in probe order. I
>> will check more on this and get back.
>
>
> Right now, it probably used the order of probing. Because iommu controllers
> are probed earlier, so they are handled before the master devices.
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
amit daniel kachhap <[email protected]> writes:
> On Wed, Dec 17, 2014 at 3:40 AM, Kevin Hilman <[email protected]> wrote:
>> Marek Szyprowski <[email protected]> writes:
>>
>>> Hello,
>>>
>>> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
>>>> Instead of using bool to restore suspended devices initially, use flags
>>>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
>>>> The first two flags will be similar to the existing true/false functionality.
>>>> The third flag may be used to force restore of suspended devices
>>>> whenever their associated power domain is turned on.
>>>>
>>>> Currently, PD power off function powers off all the associated unused
>>>> devices. The functionality added in this patch is similar to it.
>>>>
>>>> This feature may be used for those devices which are always in ON state
>>>> if the PD associated with them is ON but may need local runtime resume
>>>> and suspend during PD On/Off. These devices (like clock) may not implement
>>>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
>>>> subsystems interaction behaviour or any other reason.
>>>>
>>>> The model works like,
>>>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
>>>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
>>>> / GPD_DEV_RESTORE_FORCE option)
>>>> /
>>>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
>>>> \ pm_runtime_put. This in turn invokes PD On/Off)
>>>> \
>>>> DEV3 (Similar to DEV1)
>>>>
>>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>>>
>>> The idea of adding new gen_pd flag and reusing runtime pm calls intead
>>> of additional notifiers looks promising, but I have some doubts.
>>
>> I agree, this is better than notifiers, but I have some doubts too.
>
> Thanks,
>
>>
>>> don't see any guarantee that devices with GPD_DEV_RESTORE_FORCE flag
>>> will be suspended after all "normal" devices and restored before
>>> them. Without such assumption it will be hard to use this approach for
>>> iommu related activities, because device might need to use (in its
>>> suspend/resume callbacks) the functionality provided by the other
>>> device with GPD_DEV_RESTORE_FORCE flag. Maybe some additional flags
>>> like suspend/resume priority (or more flags) will solve somehow this
>>> dependency.
>>
>> At a deeper level, the problem with this approach is that this is more
>> generically a runtime PM dependency problem, not a genpd problem. For
>> example, what happens when the same kind of dependency exists on a
>> platform using a custom PM domain instead of genpd (like ACPI.) ?
>
> This patch does not try to solve runtime PM dependencies between
> devices. As an example, if there are three devices D1, D2, D3 in a
> power domain. Device D3 would update the power domain state
> requirement using runtime PM API but devices D1 and D2 do not want to
> control the domain but just want to be notified when the power domain
> state changes.
Yes, I understand that.
The question is: what do you do when you have the same dependency
problem and you're not using genpd (for example, some SoCs have
implmeented their own PM domains, and ACPI devices are managed by their
own PM domain, not genpd.)
>> What's needed to solve this problem is a generalized way to have runtime
>> PM dependencies between devices. Runtime PM already automatically
>> handles parent devices as one type of dependent device (e.g. a parent
>> device needs to be runtime PM resumed before its child.) So what's
>> needed is a generic way to other PM dependencies with the runtime PM
>> core (not the genpd core.)
>
> Considering the example above with three devices, device D1 and D2 are
> passive components in this power domain. These devices only need to
> know the state changes of the power domains but would not control the
> power domain themselves nor put forth constraints in the power domain
> state changes. So I did not clearly understand as to how this example
> could be solved by introducing changes in runtime PM core.
Your solution only solves the problems for devices managed by genpd.
If I understood your example correctly, what you really want to solve
this problem more generically is to be able to tell the runtime PM core
that D3 has a dependency on D1 and D2. Then, whenver the runtime PM
core is doing get/put operations for D3, it needs to also do them for D1
and D2.
This will accomplish the same as your proposed approach, but work for
any devices in any PM domains.
Kevin
On Wednesday, December 17, 2014 10:25:09 AM Kevin Hilman wrote:
> amit daniel kachhap <[email protected]> writes:
>
> > On Wed, Dec 17, 2014 at 3:40 AM, Kevin Hilman <[email protected]> wrote:
> >> Marek Szyprowski <[email protected]> writes:
> >>
> >>> Hello,
> >>>
> >>> On 2014-12-13 17:51, Amit Daniel Kachhap wrote:
> >>>> Instead of using bool to restore suspended devices initially, use flags
> >>>> like GPD_DEV_SUSPEND_INIT, GPD_DEV_RESTORE_INIT and GPD_DEV_RESTORE_FORCE.
> >>>> The first two flags will be similar to the existing true/false functionality.
> >>>> The third flag may be used to force restore of suspended devices
> >>>> whenever their associated power domain is turned on.
> >>>>
> >>>> Currently, PD power off function powers off all the associated unused
> >>>> devices. The functionality added in this patch is similar to it.
> >>>>
> >>>> This feature may be used for those devices which are always in ON state
> >>>> if the PD associated with them is ON but may need local runtime resume
> >>>> and suspend during PD On/Off. These devices (like clock) may not implement
> >>>> complete pm_runtime calls such as pm_runtime_get/pm_runtime_put due to
> >>>> subsystems interaction behaviour or any other reason.
> >>>>
> >>>> The model works like,
> >>>> DEV1 (Attaches itself with PD but no calls to pm_runtime_get and
> >>>> / pm_runtime_put. Its local runtime_suspend/resume is invoked via
> >>>> / GPD_DEV_RESTORE_FORCE option)
> >>>> /
> >>>> PD -- DEV2 (Implements complete PM runtime and calls pm_runtime_get and
> >>>> \ pm_runtime_put. This in turn invokes PD On/Off)
> >>>> \
> >>>> DEV3 (Similar to DEV1)
> >>>>
> >>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
> >>>
> >>> The idea of adding new gen_pd flag and reusing runtime pm calls intead
> >>> of additional notifiers looks promising, but I have some doubts.
> >>
> >> I agree, this is better than notifiers, but I have some doubts too.
> >
> > Thanks,
> >
> >>
> >>> don't see any guarantee that devices with GPD_DEV_RESTORE_FORCE flag
> >>> will be suspended after all "normal" devices and restored before
> >>> them. Without such assumption it will be hard to use this approach for
> >>> iommu related activities, because device might need to use (in its
> >>> suspend/resume callbacks) the functionality provided by the other
> >>> device with GPD_DEV_RESTORE_FORCE flag. Maybe some additional flags
> >>> like suspend/resume priority (or more flags) will solve somehow this
> >>> dependency.
> >>
> >> At a deeper level, the problem with this approach is that this is more
> >> generically a runtime PM dependency problem, not a genpd problem. For
> >> example, what happens when the same kind of dependency exists on a
> >> platform using a custom PM domain instead of genpd (like ACPI.) ?
> >
> > This patch does not try to solve runtime PM dependencies between
> > devices. As an example, if there are three devices D1, D2, D3 in a
> > power domain. Device D3 would update the power domain state
> > requirement using runtime PM API but devices D1 and D2 do not want to
> > control the domain but just want to be notified when the power domain
> > state changes.
>
> Yes, I understand that.
>
> The question is: what do you do when you have the same dependency
> problem and you're not using genpd (for example, some SoCs have
> implmeented their own PM domains, and ACPI devices are managed by their
> own PM domain, not genpd.)
>
> >> What's needed to solve this problem is a generalized way to have runtime
> >> PM dependencies between devices. Runtime PM already automatically
> >> handles parent devices as one type of dependent device (e.g. a parent
> >> device needs to be runtime PM resumed before its child.) So what's
> >> needed is a generic way to other PM dependencies with the runtime PM
> >> core (not the genpd core.)
> >
> > Considering the example above with three devices, device D1 and D2 are
> > passive components in this power domain. These devices only need to
> > know the state changes of the power domains but would not control the
> > power domain themselves nor put forth constraints in the power domain
> > state changes. So I did not clearly understand as to how this example
> > could be solved by introducing changes in runtime PM core.
>
> Your solution only solves the problems for devices managed by genpd.
>
> If I understood your example correctly, what you really want to solve
> this problem more generically is to be able to tell the runtime PM core
> that D3 has a dependency on D1 and D2. Then, whenver the runtime PM
> core is doing get/put operations for D3, it needs to also do them for D1
> and D2.
>
> This will accomplish the same as your proposed approach, but work for
> any devices in any PM domains.
Plus, it is not limited to runtime PM, really. It affects system suspend
too.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 18/12/14 01:58, Rafael J. Wysocki wrote:
>>>> What's needed to solve this problem is a generalized way to have runtime
>>>> > >> PM dependencies between devices. Runtime PM already automatically
>>>> > >> handles parent devices as one type of dependent device (e.g. a parent
>>>> > >> device needs to be runtime PM resumed before its child.) So what's
>>>> > >> needed is a generic way to other PM dependencies with the runtime PM
>>>> > >> core (not the genpd core.)
>>> > >
>>> > > Considering the example above with three devices, device D1 and D2 are
>>> > > passive components in this power domain. These devices only need to
>>> > > know the state changes of the power domains but would not control the
>>> > > power domain themselves nor put forth constraints in the power domain
>>> > > state changes. So I did not clearly understand as to how this example
>>> > > could be solved by introducing changes in runtime PM core.
>> >
>> > Your solution only solves the problems for devices managed by genpd.
>> >
>> > If I understood your example correctly, what you really want to solve
>> > this problem more generically is to be able to tell the runtime PM core
>> > that D3 has a dependency on D1 and D2. Then, whenver the runtime PM
>> > core is doing get/put operations for D3, it needs to also do them for D1
>> > and D2.
Indeed, I think it would solve most of the problems if we were able to
model the PM dependencies between devices which would then be handled
in the PM core. I recall something like this has been proposed a while
ago [1].
>> > This will accomplish the same as your proposed approach, but work for
>> > any devices in any PM domains.
>
> Plus, it is not limited to runtime PM, really. It affects system suspend
> too.
[1] https://lkml.org/lkml/2009/8/26/485
--
Regards,
Sylwester
On Thursday, December 18, 2014 11:05:18 AM Sylwester Nawrocki wrote:
> On 18/12/14 01:58, Rafael J. Wysocki wrote:
> >>>> What's needed to solve this problem is a generalized way to have runtime
> >>>> > >> PM dependencies between devices. Runtime PM already automatically
> >>>> > >> handles parent devices as one type of dependent device (e.g. a parent
> >>>> > >> device needs to be runtime PM resumed before its child.) So what's
> >>>> > >> needed is a generic way to other PM dependencies with the runtime PM
> >>>> > >> core (not the genpd core.)
> >>> > >
> >>> > > Considering the example above with three devices, device D1 and D2 are
> >>> > > passive components in this power domain. These devices only need to
> >>> > > know the state changes of the power domains but would not control the
> >>> > > power domain themselves nor put forth constraints in the power domain
> >>> > > state changes. So I did not clearly understand as to how this example
> >>> > > could be solved by introducing changes in runtime PM core.
> >> >
> >> > Your solution only solves the problems for devices managed by genpd.
> >> >
> >> > If I understood your example correctly, what you really want to solve
> >> > this problem more generically is to be able to tell the runtime PM core
> >> > that D3 has a dependency on D1 and D2. Then, whenver the runtime PM
> >> > core is doing get/put operations for D3, it needs to also do them for D1
> >> > and D2.
>
> Indeed, I think it would solve most of the problems if we were able to
> model the PM dependencies between devices which would then be handled
> in the PM core. I recall something like this has been proposed a while
> ago [1].
Exactly.
And I'm going to revive it in a slightly simplified form.
> >> > This will accomplish the same as your proposed approach, but work for
> >> > any devices in any PM domains.
> >
> > Plus, it is not limited to runtime PM, really. It affects system suspend
> > too.
>
> [1] https://lkml.org/lkml/2009/8/26/485
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.