[03/12] PM / Domains: Add notifier support for power domain transitions

Commit Message

Amit Kachhap Nov. 3, 2014, 3:53 a.m. UTC

These power domain transition notifiers will assist in carrying
out some activity associated with domain power on/off such as
some registers which may lose its contents and need save/restore
across domain power off/on.

4 type of notifications are added,
GPD_OFF_PRE     - GPD state before power off
GPD_OFF_POST    - GPD state after power off
GPD_ON_PRE      - GPD state before power off
GPD_ON_POST     - GPD state after power off

3 notfication API's are exported.
1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
			enum gpd_on_off_state state);

In this approach the notifiers are registered/unregistered with pd name.
The actual invoking of the notfiers will be done by the platform implementing
power domain enable/disable low level handlers according to the above
defined notification types. This approach will considerably reduce the
number of call to notifiers as compared to calling always from core
powerdomain subsystem.

Also the registered domain's will be managed inside a cache list and not
part of the genpd structure. This will help in registration of notifiers from
subsystems (such as clock) even when the PD subsystem is still not initialised.
This list also filters out the unregistered pd's requesting notification.

Cc: Rafael J. Wysocki <[e-mail]>
Reviewed-by: Pankaj Dubey <[e-mail]>
Signed-off-by: Amit Daniel Kachhap <[e-mail]>
---
 drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pm_domain.h   |   31 ++++++++++++
 2 files changed, 142 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Nov. 3, 2014, 2:52 p.m. UTC | #1

On 3 November 2014 15:54, Rafael J. Wysocki <[e-mail]> wrote:
> CC: Kevin, Ulf, Geert.
>
> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>> These power domain transition notifiers will assist in carrying
>> out some activity associated with domain power on/off such as
>> some registers which may lose its contents and need save/restore
>> across domain power off/on.

Hi Amit,

This mechanism seems to be needed - somehow.
A similar approach has been posted earlier, I am not sure that
discussion was fully settled though.

http://marc.info/?l=linux-pm&m=136448756308203&w=2

Anyway, I plan to review this shortly.

Kind regards
Uffe

>>
>> 4 type of notifications are added,
>> GPD_OFF_PRE     - GPD state before power off
>> GPD_OFF_POST    - GPD state after power off
>> GPD_ON_PRE      - GPD state before power off
>> GPD_ON_POST     - GPD state after power off
>>
>> 3 notfication API's are exported.
>> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>                       enum gpd_on_off_state state);
>>
>> In this approach the notifiers are registered/unregistered with pd name.
>> The actual invoking of the notfiers will be done by the platform implementing
>> power domain enable/disable low level handlers according to the above
>> defined notification types. This approach will considerably reduce the
>> number of call to notifiers as compared to calling always from core
>> powerdomain subsystem.
>>
>> Also the registered domain's will be managed inside a cache list and not
>> part of the genpd structure. This will help in registration of notifiers from
>> subsystems (such as clock) even when the PD subsystem is still not initialised.
>> This list also filters out the unregistered pd's requesting notification.
>>
>> Cc: Rafael J. Wysocki <[e-mail]>
>> Reviewed-by: Pankaj Dubey <[e-mail]>
>> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
>> ---
>>  drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/pm_domain.h   |   31 ++++++++++++
>>  2 files changed, 142 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 40bc2f4..e05045e 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -46,10 +46,19 @@
>>       __retval;                                                               \
>>  })
>>
>> +struct cache_notify_domains {
>> +     char *name;
>> +     /* Node in the cache pm domain name list */
>> +     struct list_head cache_list_node;
>> +};
>> +
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>> +static LIST_HEAD(domain_notify_list);
>> +static DEFINE_MUTEX(domain_notify_list_lock);
>> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
>>
>> -static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>  {
>>       struct generic_pm_domain *genpd = NULL, *gpd;
>>
>> @@ -66,6 +75,7 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>       mutex_unlock(&gpd_list_lock);
>>       return genpd;
>>  }
>> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
>>
>>  struct generic_pm_domain *dev_to_genpd(struct device *dev)
>>  {
>> @@ -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>       mutex_unlock(&gpd_list_lock);
>>  }
>>
>> +/**
>> + * genpd_register_notifier - Register a PM domain for future notification.
>> + * @nb: notification block containing notification handle.
>> + * @pd_name: PM domain name.
>> + */
>> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +     int ret;
>> +     struct cache_notify_domains *entry;
>> +
>> +     if (!pd_name)
>> +             return -EINVAL;
>> +
>> +     /* Search if the pd is already registered */
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +             if (!strcmp(entry->name, pd_name))
>> +                     break;
>> +     }
>> +     mutex_unlock(&domain_notify_list_lock);
>> +
>> +     if (entry) {
>> +             pr_err("%s: pd already exists\n", __func__);
>> +             return -EINVAL;
>> +     }
>> +
>> +     entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> +     if (!entry)
>> +             return -ENOMEM;
>> +
>> +     entry->name = pd_name;
>> +
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_add(&entry->cache_list_node, &domain_notify_list);
>> +     mutex_unlock(&domain_notify_list_lock);
>> +     ret = blocking_notifier_chain_register(
>> +                             &genpd_transition_notifier_list, nb);
>> +     return ret;
>> +}
>> +
>> +/**
>> + * genpd_unregister_notifier - Un-register a PM domain for future notification.
>> + * @nb: notification block containing notification handle.
>> + * @pd_name: PM domain name.
>> + */
>> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +     int ret;
>> +     struct cache_notify_domains *entry;
>> +
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +             if (!strcmp(entry->name, pd_name))
>> +                     break;
>> +     }
>> +     if (!entry) {
>> +             mutex_unlock(&domain_notify_list_lock);
>> +             pr_err("%s: Invalid pd name\n", __func__);
>> +             return -EINVAL;
>> +     }
>> +     list_del(&entry->cache_list_node);
>> +     mutex_unlock(&domain_notify_list_lock);
>> +     ret = blocking_notifier_chain_unregister(
>> +                             &genpd_transition_notifier_list, nb);
>> +     return ret;
>> +}
>> +
>> +/**
>> + * genpd_invoke_transition_notifier - Calls the matching notification handler.
>> + * @genpd: generic power domain structure.
>> + * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
>> + */
>> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>> +                     enum gpd_on_off_state state)
>> +{
>> +     struct cache_notify_domains *entry;
>> +
>> +     if (!genpd) {
>> +             pr_err("Invalid genpd parameter\n");
>> +             return;
>> +     }
>> +
>> +     if (state != GPD_OFF_PRE || state != GPD_OFF_POST
>> +                     || state != GPD_ON_PRE || state != GPD_ON_POST) {
>> +             pr_err("Invalid state parameter\n");
>> +             return;
>> +     }
>> +
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +             if (!strcmp(entry->name, genpd->name))
>> +                     break;
>> +     }
>> +     mutex_unlock(&domain_notify_list_lock);
>> +     if (!entry) /* Simply ignore */
>> +             return;
>> +
>> +     blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
>> +                             genpd);
>> +}
>>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>>  /*
>>   * Device Tree based PM domain providers.
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 73e938b..659997f 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -25,6 +25,13 @@ enum gpd_status {
>>       GPD_STATE_POWER_OFF,    /* PM domain is off */
>>  };
>>
>> +enum gpd_on_off_state {
>> +     GPD_OFF_PRE = 0,        /* GPD state before power off */
>> +     GPD_OFF_POST,           /* GPD state after power off */
>> +     GPD_ON_PRE,             /* GPD state before power on */
>> +     GPD_ON_POST,            /* GPD state after power on */
>> +};
>> +
>>  struct dev_power_governor {
>>       bool (*power_down_ok)(struct dev_pm_domain *domain);
>>       bool (*stop_ok)(struct device *dev);
>> @@ -148,6 +155,12 @@ extern int pm_genpd_name_poweron(const char *domain_name);
>>
>>  extern struct dev_power_governor simple_qos_governor;
>>  extern struct dev_power_governor pm_domain_always_on_gov;
>> +
>> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name);
>> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>> +                     enum gpd_on_off_state state);
>>  #else
>>
>>  static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>> @@ -219,6 +232,24 @@ static inline int pm_genpd_name_poweron(const char *domain_name)
>>  {
>>       return -ENOSYS;
>>  }
>> +static inline struct
>> +generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>> +{
>> +     return NULL;
>> +}
>> +static inline int
>> +genpd_register_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +     return 0;
>> +}
>> +static inline int
>> +genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +     return 0;
>> +}
>> +static inline void
>> +genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>> +                     enum gpd_on_off_state state) { }
>>  #define simple_qos_governor NULL
>>  #define pm_domain_always_on_gov NULL
>>  #endif
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki Nov. 3, 2014, 2:54 p.m. UTC | #2

CC: Kevin, Ulf, Geert.

On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
> These power domain transition notifiers will assist in carrying
> out some activity associated with domain power on/off such as
> some registers which may lose its contents and need save/restore
> across domain power off/on.
> 
> 4 type of notifications are added,
> GPD_OFF_PRE     - GPD state before power off
> GPD_OFF_POST    - GPD state after power off
> GPD_ON_PRE      - GPD state before power off
> GPD_ON_POST     - GPD state after power off
> 
> 3 notfication API's are exported.
> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> 			enum gpd_on_off_state state);
> 
> In this approach the notifiers are registered/unregistered with pd name.
> The actual invoking of the notfiers will be done by the platform implementing
> power domain enable/disable low level handlers according to the above
> defined notification types. This approach will considerably reduce the
> number of call to notifiers as compared to calling always from core
> powerdomain subsystem.
> 
> Also the registered domain's will be managed inside a cache list and not
> part of the genpd structure. This will help in registration of notifiers from
> subsystems (such as clock) even when the PD subsystem is still not initialised.
> This list also filters out the unregistered pd's requesting notification.
> 
> Cc: Rafael J. Wysocki <[e-mail]>
> Reviewed-by: Pankaj Dubey <[e-mail]>
> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
> ---
>  drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_domain.h   |   31 ++++++++++++
>  2 files changed, 142 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 40bc2f4..e05045e 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -46,10 +46,19 @@
>  	__retval;								\
>  })
>  
> +struct cache_notify_domains {
> +	char *name;
> +	/* Node in the cache pm domain name list */
> +	struct list_head cache_list_node;
> +};
> +
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
> +static LIST_HEAD(domain_notify_list);
> +static DEFINE_MUTEX(domain_notify_list_lock);
> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
>  
> -static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>  {
>  	struct generic_pm_domain *genpd = NULL, *gpd;
>  
> @@ -66,6 +75,7 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>  	mutex_unlock(&gpd_list_lock);
>  	return genpd;
>  }
> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
>  
>  struct generic_pm_domain *dev_to_genpd(struct device *dev)
>  {
> @@ -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>  	mutex_unlock(&gpd_list_lock);
>  }
>  
> +/**
> + * genpd_register_notifier - Register a PM domain for future notification.
> + * @nb: notification block containing notification handle.
> + * @pd_name: PM domain name.
> + */
> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
> +{
> +	int ret;
> +	struct cache_notify_domains *entry;
> +
> +	if (!pd_name)
> +		return -EINVAL;
> +
> +	/* Search if the pd is already registered */
> +	mutex_lock(&domain_notify_list_lock);
> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> +		if (!strcmp(entry->name, pd_name))
> +			break;
> +	}
> +	mutex_unlock(&domain_notify_list_lock);
> +
> +	if (entry) {
> +		pr_err("%s: pd already exists\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	entry->name = pd_name;
> +
> +	mutex_lock(&domain_notify_list_lock);
> +	list_add(&entry->cache_list_node, &domain_notify_list);
> +	mutex_unlock(&domain_notify_list_lock);
> +	ret = blocking_notifier_chain_register(
> +				&genpd_transition_notifier_list, nb);
> +	return ret;
> +}
> +
> +/**
> + * genpd_unregister_notifier - Un-register a PM domain for future notification.
> + * @nb: notification block containing notification handle.
> + * @pd_name: PM domain name.
> + */
> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
> +{
> +	int ret;
> +	struct cache_notify_domains *entry;
> +
> +	mutex_lock(&domain_notify_list_lock);
> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> +		if (!strcmp(entry->name, pd_name))
> +			break;
> +	}
> +	if (!entry) {
> +		mutex_unlock(&domain_notify_list_lock);
> +		pr_err("%s: Invalid pd name\n", __func__);
> +		return -EINVAL;
> +	}
> +	list_del(&entry->cache_list_node);
> +	mutex_unlock(&domain_notify_list_lock);
> +	ret = blocking_notifier_chain_unregister(
> +				&genpd_transition_notifier_list, nb);
> +	return ret;
> +}
> +
> +/**
> + * genpd_invoke_transition_notifier - Calls the matching notification handler.
> + * @genpd: generic power domain structure.
> + * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
> + */
> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> +			enum gpd_on_off_state state)
> +{
> +	struct cache_notify_domains *entry;
> +
> +	if (!genpd) {
> +		pr_err("Invalid genpd parameter\n");
> +		return;
> +	}
> +
> +	if (state != GPD_OFF_PRE || state != GPD_OFF_POST
> +			|| state != GPD_ON_PRE || state != GPD_ON_POST) {
> +		pr_err("Invalid state parameter\n");
> +		return;
> +	}
> +
> +	mutex_lock(&domain_notify_list_lock);
> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> +		if (!strcmp(entry->name, genpd->name))
> +			break;
> +	}
> +	mutex_unlock(&domain_notify_list_lock);
> +	if (!entry) /* Simply ignore */
> +		return;
> +
> +	blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
> +				genpd);
> +}
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  /*
>   * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 73e938b..659997f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -25,6 +25,13 @@ enum gpd_status {
>  	GPD_STATE_POWER_OFF,	/* PM domain is off */
>  };
>  
> +enum gpd_on_off_state {
> +	GPD_OFF_PRE = 0,	/* GPD state before power off */
> +	GPD_OFF_POST,		/* GPD state after power off */
> +	GPD_ON_PRE,		/* GPD state before power on */
> +	GPD_ON_POST,		/* GPD state after power on */
> +};
> +
>  struct dev_power_governor {
>  	bool (*power_down_ok)(struct dev_pm_domain *domain);
>  	bool (*stop_ok)(struct device *dev);
> @@ -148,6 +155,12 @@ extern int pm_genpd_name_poweron(const char *domain_name);
>  
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> +
> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name);
> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> +			enum gpd_on_off_state state);
>  #else
>  
>  static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
> @@ -219,6 +232,24 @@ static inline int pm_genpd_name_poweron(const char *domain_name)
>  {
>  	return -ENOSYS;
>  }
> +static inline struct
> +generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> +{
> +	return NULL;
> +}
> +static inline int
> +genpd_register_notifier(struct notifier_block *nb, char *pd_name)
> +{
> +	return 0;
> +}
> +static inline int
> +genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
> +{
> +	return 0;
> +}
> +static inline void
> +genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> +			enum gpd_on_off_state state) { }
>  #define simple_qos_governor NULL
>  #define pm_domain_always_on_gov NULL
>  #endif
>

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 3, 2014, 6:21 p.m. UTC | #3

Hi,

Cc: Ulf, Kevin, Geert.

On 03/11/14 04:53, Amit Daniel Kachhap wrote:
> These power domain transition notifiers will assist in carrying
> out some activity associated with domain power on/off such as
> some registers which may lose its contents and need save/restore
> across domain power off/on.

Other specific use cases I could add here is gating selected clocks
to ensure proper signal propagation for devices attached to a power
domain, e.g. ungating selected clocks before the power domain on and
restoring them to previous state after the power domain was switched
on.

> 4 type of notifications are added,
> GPD_OFF_PRE     - GPD state before power off
> GPD_OFF_POST    - GPD state after power off
> GPD_ON_PRE      - GPD state before power off
> GPD_ON_POST     - GPD state after power off
> 
> 3 notfication API's are exported.
> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> 			enum gpd_on_off_state state);
> 
> In this approach the notifiers are registered/unregistered with pd name.
> The actual invoking of the notfiers will be done by the platform implementing
> power domain enable/disable low level handlers according to the above
> defined notification types. This approach will considerably reduce the
> number of call to notifiers as compared to calling always from core
> powerdomain subsystem.
> 
> Also the registered domain's will be managed inside a cache list and not
> part of the genpd structure. This will help in registration of notifiers from
> subsystems (such as clock) even when the PD subsystem is still not initialised.
> This list also filters out the unregistered pd's requesting notification.

This patch is somewhat similar my patch adding power domain state change
notifications [1].  I have already a reworked version of that patch, with the
notifier list moved to struct generic_pm_domain and using genpd->lock instead
of dev->power.lock.  Anyway, while I'd leave the decision about the location
from where the notifier chains are supposed to be called to the subsystem's
maintainers preference, I'm rather reluctant to having one global notifiers
list for all possible power domains and all the notification clients.
The possibly long list needs to be traversed at each notifier call and it
seems in your implementation any registered user of the notification gets
notifications for all possible power domains.

[1] https://lkml.org/lkml/2014/8/5/182

> Cc: Rafael J. Wysocki <[e-mail]>
> Reviewed-by: Pankaj Dubey <[e-mail]>
> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
> ---
>  drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_domain.h   |   31 ++++++++++++
>  2 files changed, 142 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 40bc2f4..e05045e 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -46,10 +46,19 @@
>  	__retval;								\
>  })
>  
> +struct cache_notify_domains {
> +	char *name;
> +	/* Node in the cache pm domain name list */
> +	struct list_head cache_list_node;
> +};
> +
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
> +static LIST_HEAD(domain_notify_list);
> +static DEFINE_MUTEX(domain_notify_list_lock);
> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
>  
> -static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>  {
>  	struct generic_pm_domain *genpd = NULL, *gpd;
>  
> @@ -66,6 +75,7 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>  	mutex_unlock(&gpd_list_lock);
>  	return genpd;
>  }
> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
>  
>  struct generic_pm_domain *dev_to_genpd(struct device *dev)
>  {
> @@ -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>  	mutex_unlock(&gpd_list_lock);
>  }
>  
> +/**
> + * genpd_register_notifier - Register a PM domain for future notification.
> + * @nb: notification block containing notification handle.
> + * @pd_name: PM domain name.
> + */
> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
> +{
> +	int ret;
> +	struct cache_notify_domains *entry;
> +
> +	if (!pd_name)
> +		return -EINVAL;
> +
> +	/* Search if the pd is already registered */
> +	mutex_lock(&domain_notify_list_lock);
> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> +		if (!strcmp(entry->name, pd_name))
> +			break;
> +	}
> +	mutex_unlock(&domain_notify_list_lock);
> +
> +	if (entry) {
> +		pr_err("%s: pd already exists\n", __func__);
> +		return -EINVAL;

I suspect this code doesn't work as expected. 'entry' will be NULL only
in case of an empty domain_notify_list list. Have you tested multiple
calls to genpd_register_notifier() ? It looks like only the first call
would work.

> +	}
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	entry->name = pd_name;
> +
> +	mutex_lock(&domain_notify_list_lock);
> +	list_add(&entry->cache_list_node, &domain_notify_list);
> +	mutex_unlock(&domain_notify_list_lock);
> +	ret = blocking_notifier_chain_register(
> +				&genpd_transition_notifier_list, nb);
> +	return ret;
> +}
> +
> +/**
> + * genpd_unregister_notifier - Un-register a PM domain for future notification.
> + * @nb: notification block containing notification handle.
> + * @pd_name: PM domain name.
> + */
> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
> +{
> +	int ret;
> +	struct cache_notify_domains *entry;
> +
> +	mutex_lock(&domain_notify_list_lock);
> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> +		if (!strcmp(entry->name, pd_name))
> +			break;
> +	}
> +	if (!entry) {
> +		mutex_unlock(&domain_notify_list_lock);
> +		pr_err("%s: Invalid pd name\n", __func__);
> +		return -EINVAL;
> +	}
> +	list_del(&entry->cache_list_node);

'entry' will not be NULL even when the requested notification entry
was not found above. In such case it looks like last entry in the
domain_notify_list list will be removed, not something we would expect.

> +	mutex_unlock(&domain_notify_list_lock);
> +	ret = blocking_notifier_chain_unregister(
> +				&genpd_transition_notifier_list, nb);
> +	return ret;
> +}
> +
> +/**
> + * genpd_invoke_transition_notifier - Calls the matching notification handler.
> + * @genpd: generic power domain structure.
> + * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
> + */
> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> +			enum gpd_on_off_state state)
> +{
> +	struct cache_notify_domains *entry;
> +
> +	if (!genpd) {
> +		pr_err("Invalid genpd parameter\n");
> +		return;
> +	}
> +
> +	if (state != GPD_OFF_PRE || state != GPD_OFF_POST
> +			|| state != GPD_ON_PRE || state != GPD_ON_POST) {
> +		pr_err("Invalid state parameter\n");
> +		return;
> +	}
> +
> +	mutex_lock(&domain_notify_list_lock);
> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> +		if (!strcmp(entry->name, genpd->name))
> +			break;
> +	}
> +	mutex_unlock(&domain_notify_list_lock);
> +	if (!entry) /* Simply ignore */

Similar issue here, this condition will only be true when the notifications
list is empty.

> +		return;
> +
> +	blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
> +				genpd);

And finally regardless of to what power domain the notification user has
registered it will get notification for each possible power domain in
the system?  Are the notification users supposed to test the 'genpd'
argument to react on a specific power domain? If so, how? I guess not by
nother strcmp() ?

> +}
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  /*
>   * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 73e938b..659997f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -25,6 +25,13 @@ enum gpd_status {
>  	GPD_STATE_POWER_OFF,	/* PM domain is off */
>  };
>  
> +enum gpd_on_off_state {
> +	GPD_OFF_PRE = 0,	/* GPD state before power off */

The assignment is not needed.

> +	GPD_OFF_POST,		/* GPD state after power off */
> +	GPD_ON_PRE,		/* GPD state before power on */
> +	GPD_ON_POST,		/* GPD state after power on */
> +};
> +
>  struct dev_power_governor {
>  	bool (*power_down_ok)(struct dev_pm_domain *domain);
>  	bool (*stop_ok)(struct device *dev);
> @@ -148,6 +155,12 @@ extern int pm_genpd_name_poweron(const char *domain_name);

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Nov. 3, 2014, 6:23 p.m. UTC | #4

+Mike Turquette

Hi Amit,

"Rafael J. Wysocki" <[e-mail]> writes:

> CC: Kevin, Ulf, Geert.
>
> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>> These power domain transition notifiers will assist in carrying
>> out some activity associated with domain power on/off such as
>> some registers which may lose its contents and need save/restore
>> across domain power off/on.

The runtime PM framework already provides callbacks that are useful for
context save/restore for devices.  Could you please describe in more
detail which registers in which kind of devices need to be
saved/restored, and why they cannot be saved/restored using existing
mechanisms.

Personally, I'm uncomfortable with notifiers like this because it
suggests that underlying frameworks are not doing the right thing, or
are not being used.  (I also don't like the implementation here where a
single global notifier list is maintained by the core, but the notifiers
are actually triggered by SoC specific code.)

IIUC, the usage in this series seems to be that certain clock related
registers need to be saved/restored across a power domain transition.

Wouldn't an alternative solution be to add a feature to the clock driver
such that the state of each clock is saved when the clock is disabled,
and restored when the clock is enabled?   That would allow any clock
context to survive any power domain transtion also, correct?

I have some issues with the implementaion as well, but I think we need
to first sort out the real need for this before going into those
details.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 3, 2014, 6:41 p.m. UTC | #5

Now really filling in the CC list, apologies for duplicate
post.

On 03/11/14 19:21, Sylwester Nawrocki wrote:
> Hi,
> 
> Cc: Ulf, Kevin, Geert.
> 
> On 03/11/14 04:53, Amit Daniel Kachhap wrote:
>> These power domain transition notifiers will assist in carrying
>> out some activity associated with domain power on/off such as
>> some registers which may lose its contents and need save/restore
>> across domain power off/on.
> 
> Other specific use cases I could add here is gating selected clocks
> to ensure proper signal propagation for devices attached to a power
> domain, e.g. ungating selected clocks before the power domain on and
> restoring them to previous state after the power domain was switched
> on.
> 
>> 4 type of notifications are added,
>> GPD_OFF_PRE     - GPD state before power off
>> GPD_OFF_POST    - GPD state after power off
>> GPD_ON_PRE      - GPD state before power off
>> GPD_ON_POST     - GPD state after power off
>>
>> 3 notfication API's are exported.
>> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>> 			enum gpd_on_off_state state);
>>
>> In this approach the notifiers are registered/unregistered with pd name.
>> The actual invoking of the notfiers will be done by the platform implementing
>> power domain enable/disable low level handlers according to the above
>> defined notification types. This approach will considerably reduce the
>> number of call to notifiers as compared to calling always from core
>> powerdomain subsystem.
>>
>> Also the registered domain's will be managed inside a cache list and not
>> part of the genpd structure. This will help in registration of notifiers from
>> subsystems (such as clock) even when the PD subsystem is still not initialised.
>> This list also filters out the unregistered pd's requesting notification.
> 
> This patch is somewhat similar my patch adding power domain state change
> notifications [1].  I have already a reworked version of that patch, with the
> notifier list moved to struct generic_pm_domain and using genpd->lock instead
> of dev->power.lock.  Anyway, while I'd leave the decision about the location
> from where the notifier chains are supposed to be called to the subsystem's
> maintainers preference, I'm rather reluctant to having one global notifiers
> list for all possible power domains and all the notification clients.
> The possibly long list needs to be traversed at each notifier call and it
> seems in your implementation any registered user of the notification gets
> notifications for all possible power domains.
> 
> [1] https://lkml.org/lkml/2014/8/5/182
> 
>> Cc: Rafael J. Wysocki <[e-mail]>
>> Reviewed-by: Pankaj Dubey <[e-mail]>
>> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
>> ---
>>  drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/pm_domain.h   |   31 ++++++++++++
>>  2 files changed, 142 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 40bc2f4..e05045e 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -46,10 +46,19 @@
>>  	__retval;								\
>>  })
>>  
>> +struct cache_notify_domains {
>> +	char *name;
>> +	/* Node in the cache pm domain name list */
>> +	struct list_head cache_list_node;
>> +};
>> +
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>> +static LIST_HEAD(domain_notify_list);
>> +static DEFINE_MUTEX(domain_notify_list_lock);
>> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
>>  
>> -static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>  {
>>  	struct generic_pm_domain *genpd = NULL, *gpd;
>>  
>> @@ -66,6 +75,7 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>  	mutex_unlock(&gpd_list_lock);
>>  	return genpd;
>>  }
>> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
>>  
>>  struct generic_pm_domain *dev_to_genpd(struct device *dev)
>>  {
>> @@ -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>  	mutex_unlock(&gpd_list_lock);
>>  }
>>  
>> +/**
>> + * genpd_register_notifier - Register a PM domain for future notification.
>> + * @nb: notification block containing notification handle.
>> + * @pd_name: PM domain name.
>> + */
>> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +	int ret;
>> +	struct cache_notify_domains *entry;
>> +
>> +	if (!pd_name)
>> +		return -EINVAL;
>> +
>> +	/* Search if the pd is already registered */
>> +	mutex_lock(&domain_notify_list_lock);
>> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +		if (!strcmp(entry->name, pd_name))
>> +			break;
>> +	}
>> +	mutex_unlock(&domain_notify_list_lock);
>> +
>> +	if (entry) {
>> +		pr_err("%s: pd already exists\n", __func__);
>> +		return -EINVAL;
> 
> I suspect this code doesn't work as expected. 'entry' will be NULL only
> in case of an empty domain_notify_list list. Have you tested multiple
> calls to genpd_register_notifier() ? It looks like only the first call
> would work.
> 
>> +	}
>> +
>> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry)
>> +		return -ENOMEM;
>> +
>> +	entry->name = pd_name;
>> +
>> +	mutex_lock(&domain_notify_list_lock);
>> +	list_add(&entry->cache_list_node, &domain_notify_list);
>> +	mutex_unlock(&domain_notify_list_lock);
>> +	ret = blocking_notifier_chain_register(
>> +				&genpd_transition_notifier_list, nb);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * genpd_unregister_notifier - Un-register a PM domain for future notification.
>> + * @nb: notification block containing notification handle.
>> + * @pd_name: PM domain name.
>> + */
>> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +	int ret;
>> +	struct cache_notify_domains *entry;
>> +
>> +	mutex_lock(&domain_notify_list_lock);
>> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +		if (!strcmp(entry->name, pd_name))
>> +			break;
>> +	}
>> +	if (!entry) {
>> +		mutex_unlock(&domain_notify_list_lock);
>> +		pr_err("%s: Invalid pd name\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +	list_del(&entry->cache_list_node);
> 
> 'entry' will not be NULL even when the requested notification entry
> was not found above. In such case it looks like last entry in the
> domain_notify_list list will be removed, not something we would expect.
> 
>> +	mutex_unlock(&domain_notify_list_lock);
>> +	ret = blocking_notifier_chain_unregister(
>> +				&genpd_transition_notifier_list, nb);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * genpd_invoke_transition_notifier - Calls the matching notification handler.
>> + * @genpd: generic power domain structure.
>> + * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
>> + */
>> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>> +			enum gpd_on_off_state state)
>> +{
>> +	struct cache_notify_domains *entry;
>> +
>> +	if (!genpd) {
>> +		pr_err("Invalid genpd parameter\n");
>> +		return;
>> +	}
>> +
>> +	if (state != GPD_OFF_PRE || state != GPD_OFF_POST
>> +			|| state != GPD_ON_PRE || state != GPD_ON_POST) {
>> +		pr_err("Invalid state parameter\n");
>> +		return;
>> +	}
>> +
>> +	mutex_lock(&domain_notify_list_lock);
>> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +		if (!strcmp(entry->name, genpd->name))
>> +			break;
>> +	}
>> +	mutex_unlock(&domain_notify_list_lock);
>> +	if (!entry) /* Simply ignore */
> 
> Similar issue here, this condition will only be true when the notifications
> list is empty.
> 
>> +		return;
>> +
>> +	blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
>> +				genpd);
> 
> And finally regardless of to what power domain the notification user has
> registered it will get notification for each possible power domain in
> the system?  Are the notification users supposed to test the 'genpd'
> argument to react on a specific power domain? If so, how? I guess not by
> nother strcmp() ?
> 
>> +}
>>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>>  /*
>>   * Device Tree based PM domain providers.
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 73e938b..659997f 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -25,6 +25,13 @@ enum gpd_status {
>>  	GPD_STATE_POWER_OFF,	/* PM domain is off */
>>  };
>>  
>> +enum gpd_on_off_state {
>> +	GPD_OFF_PRE = 0,	/* GPD state before power off */
> 
> The assignment is not needed.
> 
>> +	GPD_OFF_POST,		/* GPD state after power off */
>> +	GPD_ON_PRE,		/* GPD state before power on */
>> +	GPD_ON_POST,		/* GPD state after power on */
>> +};
>> +
>>  struct dev_power_governor {
>>  	bool (*power_down_ok)(struct dev_pm_domain *domain);
>>  	bool (*stop_ok)(struct device *dev);
>> @@ -148,6 +155,12 @@ extern int pm_genpd_name_poweron(const char *domain_name);
> 
> --
> Regards,
> Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pankaj Dubey Nov. 4, 2014, 3:23 a.m. UTC | #6

Hi,

On  Tuesday, November 04, 2014 12:11 AM, Sylwester Nawrocki wrote,
> To: [e-mail]
> Cc: Amit Daniel Kachhap; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
Rafael
> J. Wysocki; Beata Michalska; [e-mail]; Kevin Hilman; Ulf
Hansson
> Subject: Re: [PATCH 03/12] PM / Domains: Add notifier support for power
domain
> transitions
> 
> 
> Now really filling in the CC list, apologies for duplicate post.
> 
> On 03/11/14 19:21, Sylwester Nawrocki wrote:
> > Hi,
> >
> > Cc: Ulf, Kevin, Geert.
> >
> > On 03/11/14 04:53, Amit Daniel Kachhap wrote:
> >> These power domain transition notifiers will assist in carrying out
> >> some activity associated with domain power on/off such as some
> >> registers which may lose its contents and need save/restore across
> >> domain power off/on.
> >
> > Other specific use cases I could add here is gating selected clocks to
> > ensure proper signal propagation for devices attached to a power
> > domain, e.g. ungating selected clocks before the power domain on and
> > restoring them to previous state after the power domain was switched
> > on.
> >
> >> 4 type of notifications are added,
> >> GPD_OFF_PRE     - GPD state before power off
> >> GPD_OFF_POST    - GPD state after power off
> >> GPD_ON_PRE      - GPD state before power off
> >> GPD_ON_POST     - GPD state after power off
> >>
> >> 3 notfication API's are exported.
> >> 1) int genpd_register_notifier(struct notifier_block *nb, char
> >> *pd_name);
> >> 2) int genpd_unregister_notifier(struct notifier_block *nb, char
> >> *pd_name);
> >> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain
*genpd,
> >> 			enum gpd_on_off_state state);
> >>
> >> In this approach the notifiers are registered/unregistered with pd
name.
> >> The actual invoking of the notfiers will be done by the platform
> >> implementing power domain enable/disable low level handlers according
> >> to the above defined notification types. This approach will
> >> considerably reduce the number of call to notifiers as compared to
> >> calling always from core powerdomain subsystem.
> >>
> >> Also the registered domain's will be managed inside a cache list and
> >> not part of the genpd structure. This will help in registration of
> >> notifiers from subsystems (such as clock) even when the PD subsystem is
still not
> initialised.
> >> This list also filters out the unregistered pd's requesting
notification.
> >
> > This patch is somewhat similar my patch adding power domain state
> > change notifications [1].  I have already a reworked version of that
> > patch, with the notifier list moved to struct generic_pm_domain and
> > using genpd->lock instead of dev->power.lock.  Anyway, while I'd leave
> > the decision about the location from where the notifier chains are
> > supposed to be called to the subsystem's maintainers preference, I'm
> > rather reluctant to having one global notifiers list for all possible
power domains
> and all the notification clients.
> > The possibly long list needs to be traversed at each notifier call and
> > it seems in your implementation any registered user of the
> > notification gets notifications for all possible power domains.
> >
> > [1] https://lkml.org/lkml/2014/8/5/182
> >
> >> Cc: Rafael J. Wysocki <[e-mail]>
> >> Reviewed-by: Pankaj Dubey <[e-mail]>
> >> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
> >> ---
> >>  drivers/base/power/domain.c |  112
> ++++++++++++++++++++++++++++++++++++++++++-
> >>  include/linux/pm_domain.h   |   31 ++++++++++++
> >>  2 files changed, 142 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/power/domain.c
> >> b/drivers/base/power/domain.c index 40bc2f4..e05045e 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -46,10 +46,19 @@
> >>  	__retval;
\
> >>  })
> >>
> >> +struct cache_notify_domains {
> >> +	char *name;
> >> +	/* Node in the cache pm domain name list */
> >> +	struct list_head cache_list_node;
> >> +};
> >> +
> >>  static LIST_HEAD(gpd_list);
> >>  static DEFINE_MUTEX(gpd_list_lock);
> >> +static LIST_HEAD(domain_notify_list); static
> >> +DEFINE_MUTEX(domain_notify_list_lock);
> >> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
> >>
> >> -static struct generic_pm_domain *pm_genpd_lookup_name(const char
> >> *domain_name)
> >> +struct generic_pm_domain *pm_genpd_lookup_name(const char
> >> +*domain_name)
> >>  {
> >>  	struct generic_pm_domain *genpd = NULL, *gpd;
> >>
> >> @@ -66,6 +75,7 @@ static struct generic_pm_domain
> *pm_genpd_lookup_name(const char *domain_name)
> >>  	mutex_unlock(&gpd_list_lock);
> >>  	return genpd;
> >>  }
> >> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
> >>
> >>  struct generic_pm_domain *dev_to_genpd(struct device *dev)  { @@
> >> -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain
*genpd,
> >>  	mutex_unlock(&gpd_list_lock);
> >>  }
> >>
> >> +/**
> >> + * genpd_register_notifier - Register a PM domain for future
notification.
> >> + * @nb: notification block containing notification handle.
> >> + * @pd_name: PM domain name.
> >> + */
> >> +int genpd_register_notifier(struct notifier_block *nb, char
> >> +*pd_name) {
> >> +	int ret;
> >> +	struct cache_notify_domains *entry;
> >> +
> >> +	if (!pd_name)
> >> +		return -EINVAL;
> >> +
> >> +	/* Search if the pd is already registered */
> >> +	mutex_lock(&domain_notify_list_lock);
> >> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> >> +		if (!strcmp(entry->name, pd_name))
> >> +			break;
> >> +	}
> >> +	mutex_unlock(&domain_notify_list_lock);
> >> +
> >> +	if (entry) {
> >> +		pr_err("%s: pd already exists\n", __func__);
> >> +		return -EINVAL;
> >
> > I suspect this code doesn't work as expected. 'entry' will be NULL
> > only in case of an empty domain_notify_list list. Have you tested
> > multiple calls to genpd_register_notifier() ? It looks like only the
> > first call would work.
> >

You are correct it should fail for multiple calls. 
Somehow we missed this in internal review. Thanks for pointing out.

> >> +	}
> >> +
> >> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> >> +	if (!entry)
> >> +		return -ENOMEM;
> >> +
> >> +	entry->name = pd_name;
> >> +
> >> +	mutex_lock(&domain_notify_list_lock);
> >> +	list_add(&entry->cache_list_node, &domain_notify_list);
> >> +	mutex_unlock(&domain_notify_list_lock);
> >> +	ret = blocking_notifier_chain_register(
> >> +				&genpd_transition_notifier_list, nb);
> >> +	return ret;
> >> +}
> >> +
> >> +/**
> >> + * genpd_unregister_notifier - Un-register a PM domain for future
notification.
> >> + * @nb: notification block containing notification handle.
> >> + * @pd_name: PM domain name.
> >> + */
> >> +int genpd_unregister_notifier(struct notifier_block *nb, char
> >> +*pd_name) {
> >> +	int ret;
> >> +	struct cache_notify_domains *entry;
> >> +
> >> +	mutex_lock(&domain_notify_list_lock);
> >> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> >> +		if (!strcmp(entry->name, pd_name))
> >> +			break;
> >> +	}
> >> +	if (!entry) {
> >> +		mutex_unlock(&domain_notify_list_lock);
> >> +		pr_err("%s: Invalid pd name\n", __func__);
> >> +		return -EINVAL;
> >> +	}
> >> +	list_del(&entry->cache_list_node);
> >
> > 'entry' will not be NULL even when the requested notification entry
> > was not found above. In such case it looks like last entry in the
> > domain_notify_list list will be removed, not something we would expect.
> >
> >> +	mutex_unlock(&domain_notify_list_lock);
> >> +	ret = blocking_notifier_chain_unregister(
> >> +				&genpd_transition_notifier_list, nb);
> >> +	return ret;
> >> +}
> >> +
> >> +/**
> >> + * genpd_invoke_transition_notifier - Calls the matching notification
handler.
> >> + * @genpd: generic power domain structure.
> >> + * @state: can be of type -
> GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
> >> + */
> >> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
> >> +			enum gpd_on_off_state state)
> >> +{
> >> +	struct cache_notify_domains *entry;
> >> +
> >> +	if (!genpd) {
> >> +		pr_err("Invalid genpd parameter\n");
> >> +		return;
> >> +	}
> >> +
> >> +	if (state != GPD_OFF_PRE || state != GPD_OFF_POST
> >> +			|| state != GPD_ON_PRE || state != GPD_ON_POST) {
> >> +		pr_err("Invalid state parameter\n");
> >> +		return;
> >> +	}
> >> +
> >> +	mutex_lock(&domain_notify_list_lock);
> >> +	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
> >> +		if (!strcmp(entry->name, genpd->name))
> >> +			break;
> >> +	}
> >> +	mutex_unlock(&domain_notify_list_lock);
> >> +	if (!entry) /* Simply ignore */
> >
> > Similar issue here, this condition will only be true when the
> > notifications list is empty.

I also think so.

> >
> >> +		return;
> >> +
> >> +	blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
> >> +				genpd);
> >
> > And finally regardless of to what power domain the notification user
> > has registered it will get notification for each possible power domain
> > in the system?  Are the notification users supposed to test the 'genpd'
> > argument to react on a specific power domain? If so, how? I guess not
> > by nother strcmp() ?
> >
> >> +}
> >>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> >>  /*
> >>   * Device Tree based PM domain providers.
> >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> >> index 73e938b..659997f 100644
> >> --- a/include/linux/pm_domain.h
> >> +++ b/include/linux/pm_domain.h
> >> @@ -25,6 +25,13 @@ enum gpd_status {
> >>  	GPD_STATE_POWER_OFF,	/* PM domain is off */
> >>  };
> >>
> >> +enum gpd_on_off_state {
> >> +	GPD_OFF_PRE = 0,	/* GPD state before power off */
> >
> > The assignment is not needed.
> >
> >> +	GPD_OFF_POST,		/* GPD state after power off */
> >> +	GPD_ON_PRE,		/* GPD state before power on */
> >> +	GPD_ON_POST,		/* GPD state after power on */
> >> +};
> >> +
> >>  struct dev_power_governor {
> >>  	bool (*power_down_ok)(struct dev_pm_domain *domain);
> >>  	bool (*stop_ok)(struct device *dev); @@ -148,6 +155,12 @@ extern
> >> int pm_genpd_name_poweron(const char *domain_name);
> >
> > --
> > Regards,
> > Sylwester

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amit Kachhap Nov. 4, 2014, 6:16 a.m. UTC | #7

On Mon, Nov 3, 2014 at 11:51 PM, Sylwester Nawrocki
<[e-mail]> wrote:
> Hi,
>
> Cc: Ulf, Kevin, Geert.
>
> On 03/11/14 04:53, Amit Daniel Kachhap wrote:
>> These power domain transition notifiers will assist in carrying
>> out some activity associated with domain power on/off such as
>> some registers which may lose its contents and need save/restore
>> across domain power off/on.
>
> Other specific use cases I could add here is gating selected clocks
> to ensure proper signal propagation for devices attached to a power
> domain, e.g. ungating selected clocks before the power domain on and
> restoring them to previous state after the power domain was switched
> on.
yes correct.
>
>> 4 type of notifications are added,
>> GPD_OFF_PRE     - GPD state before power off
>> GPD_OFF_POST    - GPD state after power off
>> GPD_ON_PRE      - GPD state before power off
>> GPD_ON_POST     - GPD state after power off
>>
>> 3 notfication API's are exported.
>> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>                       enum gpd_on_off_state state);
>>
>> In this approach the notifiers are registered/unregistered with pd name.
>> The actual invoking of the notfiers will be done by the platform implementing
>> power domain enable/disable low level handlers according to the above
>> defined notification types. This approach will considerably reduce the
>> number of call to notifiers as compared to calling always from core
>> powerdomain subsystem.
>>
>> Also the registered domain's will be managed inside a cache list and not
>> part of the genpd structure. This will help in registration of notifiers from
>> subsystems (such as clock) even when the PD subsystem is still not initialised.
>> This list also filters out the unregistered pd's requesting notification.
>
> This patch is somewhat similar my patch adding power domain state change
> notifications [1].  I have already a reworked version of that patch, with the
> notifier list moved to struct generic_pm_domain and using genpd->lock instead

Yes this will be correct as others also suggested to make per genpd
notifier block.

> of dev->power.lock.  Anyway, while I'd leave the decision about the location
> from where the notifier chains are supposed to be called to the subsystem's
> maintainers preference, I'm rather reluctant to having one global notifiers
> list for all possible power domains and all the notification clients.
> The possibly long list needs to be traversed at each notifier call and it
> seems in your implementation any registered user of the notification gets
> notifications for all possible power domains.
>
> [1] https://lkml.org/lkml/2014/8/5/182
My fault, I somehow missed this link earlier. After going through
this, I found it registers genpd from the platform driver, so the
function signature is
int pm_genpd_register_notifier(struct device *dev, struct notifier_block *nb);
I suggest to make the function signature to be like,
int pm_genpd_register_notifier(struct device_node *np, struct
notifier_block *nb)

In this way this function should should be able to support both
platform devices and non platform devices like clk.

The function may work like,

pdev = of_find_device_by_node(np);
if (pdev) {
         //  get genpd from device and go ahead with notfier registration.
         // blocking_notifier_chain_register(genpd->pd_notifier, nb)
} else
{
        // get pd_handle from np
        // get the pd_name from phandle and try registering this gen pd
        // if the genpd subsystem is not initialised then add this in
a temporary list and register the notifier later
}

Can you post your implementation with 1st part ? Later I can post the
else part with my changes.

>
>> Cc: Rafael J. Wysocki <[e-mail]>
>> Reviewed-by: Pankaj Dubey <[e-mail]>
>> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
>> ---
>>  drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/pm_domain.h   |   31 ++++++++++++
>>  2 files changed, 142 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 40bc2f4..e05045e 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -46,10 +46,19 @@
>>       __retval;                                                               \
>>  })
>>
>> +struct cache_notify_domains {
>> +     char *name;
>> +     /* Node in the cache pm domain name list */
>> +     struct list_head cache_list_node;
>> +};
>> +
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>> +static LIST_HEAD(domain_notify_list);
>> +static DEFINE_MUTEX(domain_notify_list_lock);
>> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
>>
>> -static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>  {
>>       struct generic_pm_domain *genpd = NULL, *gpd;
>>
>> @@ -66,6 +75,7 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>       mutex_unlock(&gpd_list_lock);
>>       return genpd;
>>  }
>> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
>>
>>  struct generic_pm_domain *dev_to_genpd(struct device *dev)
>>  {
>> @@ -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>       mutex_unlock(&gpd_list_lock);
>>  }
>>
>> +/**
>> + * genpd_register_notifier - Register a PM domain for future notification.
>> + * @nb: notification block containing notification handle.
>> + * @pd_name: PM domain name.
>> + */
>> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +     int ret;
>> +     struct cache_notify_domains *entry;
>> +
>> +     if (!pd_name)
>> +             return -EINVAL;
>> +
>> +     /* Search if the pd is already registered */
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +             if (!strcmp(entry->name, pd_name))
>> +                     break;
>> +     }
>> +     mutex_unlock(&domain_notify_list_lock);
>> +
>> +     if (entry) {
>> +             pr_err("%s: pd already exists\n", __func__);
>> +             return -EINVAL;
>
> I suspect this code doesn't work as expected. 'entry' will be NULL only
> in case of an empty domain_notify_list list. Have you tested multiple
> calls to genpd_register_notifier() ? It looks like only the first call
> would work.
Yes this list check is not correct. Will update this. Thanks.
>
>> +     }
>> +
>> +     entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>> +     if (!entry)
>> +             return -ENOMEM;
>> +
>> +     entry->name = pd_name;
>> +
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_add(&entry->cache_list_node, &domain_notify_list);
>> +     mutex_unlock(&domain_notify_list_lock);
>> +     ret = blocking_notifier_chain_register(
>> +                             &genpd_transition_notifier_list, nb);
>> +     return ret;
>> +}
>> +
>> +/**
>> + * genpd_unregister_notifier - Un-register a PM domain for future notification.
>> + * @nb: notification block containing notification handle.
>> + * @pd_name: PM domain name.
>> + */
>> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
>> +{
>> +     int ret;
>> +     struct cache_notify_domains *entry;
>> +
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +             if (!strcmp(entry->name, pd_name))
>> +                     break;
>> +     }
>> +     if (!entry) {
>> +             mutex_unlock(&domain_notify_list_lock);
>> +             pr_err("%s: Invalid pd name\n", __func__);
>> +             return -EINVAL;
>> +     }
>> +     list_del(&entry->cache_list_node);
>
> 'entry' will not be NULL even when the requested notification entry
> was not found above. In such case it looks like last entry in the
> domain_notify_list list will be removed, not something we would expect.
>
>> +     mutex_unlock(&domain_notify_list_lock);
>> +     ret = blocking_notifier_chain_unregister(
>> +                             &genpd_transition_notifier_list, nb);
>> +     return ret;
>> +}
>> +
>> +/**
>> + * genpd_invoke_transition_notifier - Calls the matching notification handler.
>> + * @genpd: generic power domain structure.
>> + * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
>> + */
>> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>> +                     enum gpd_on_off_state state)
>> +{
>> +     struct cache_notify_domains *entry;
>> +
>> +     if (!genpd) {
>> +             pr_err("Invalid genpd parameter\n");
>> +             return;
>> +     }
>> +
>> +     if (state != GPD_OFF_PRE || state != GPD_OFF_POST
>> +                     || state != GPD_ON_PRE || state != GPD_ON_POST) {
>> +             pr_err("Invalid state parameter\n");
>> +             return;
>> +     }
>> +
>> +     mutex_lock(&domain_notify_list_lock);
>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>> +             if (!strcmp(entry->name, genpd->name))
>> +                     break;
>> +     }
>> +     mutex_unlock(&domain_notify_list_lock);
>> +     if (!entry) /* Simply ignore */
>
> Similar issue here, this condition will only be true when the notifications
> list is empty.
>
>> +             return;
>> +
>> +     blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
>> +                             genpd);
>
> And finally regardless of to what power domain the notification user has
> registered it will get notification for each possible power domain in
> the system?  Are the notification users supposed to test the 'genpd'
> argument to react on a specific power domain? If so, how? I guess not by
> nother strcmp() ?
>
>> +}
>>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>>  /*
>>   * Device Tree based PM domain providers.
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 73e938b..659997f 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -25,6 +25,13 @@ enum gpd_status {
>>       GPD_STATE_POWER_OFF,    /* PM domain is off */
>>  };
>>
>> +enum gpd_on_off_state {
>> +     GPD_OFF_PRE = 0,        /* GPD state before power off */
>
> The assignment is not needed.
yes correct.

Regards,
Amit
>
>> +     GPD_OFF_POST,           /* GPD state after power off */
>> +     GPD_ON_PRE,             /* GPD state before power on */
>> +     GPD_ON_POST,            /* GPD state after power on */
>> +};
>> +
>>  struct dev_power_governor {
>>       bool (*power_down_ok)(struct dev_pm_domain *domain);
>>       bool (*stop_ok)(struct device *dev);
>> @@ -148,6 +155,12 @@ extern int pm_genpd_name_poweron(const char *domain_name);
>
> --
> Regards,
> Sylwester
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amit Kachhap Nov. 4, 2014, 6:18 a.m. UTC | #8

On Mon, Nov 3, 2014 at 8:22 PM, Ulf Hansson <[e-mail]> wrote:
> On 3 November 2014 15:54, Rafael J. Wysocki <[e-mail]> wrote:
>> CC: Kevin, Ulf, Geert.
>>
>> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>>> These power domain transition notifiers will assist in carrying
>>> out some activity associated with domain power on/off such as
>>> some registers which may lose its contents and need save/restore
>>> across domain power off/on.
>
> Hi Amit,
>
> This mechanism seems to be needed - somehow.
> A similar approach has been posted earlier, I am not sure that
> discussion was fully settled though.
>
> http://marc.info/?l=linux-pm&m=136448756308203&w=2
Missed looking into it. Thanks for the pointer.
>
> Anyway, I plan to review this shortly.
Sure.
>
> Kind regards
> Uffe
>
>>>
>>> 4 type of notifications are added,
>>> GPD_OFF_PRE     - GPD state before power off
>>> GPD_OFF_POST    - GPD state after power off
>>> GPD_ON_PRE      - GPD state before power off
>>> GPD_ON_POST     - GPD state after power off
>>>
>>> 3 notfication API's are exported.
>>> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>>> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>>> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>>                       enum gpd_on_off_state state);
>>>
>>> In this approach the notifiers are registered/unregistered with pd name.
>>> The actual invoking of the notfiers will be done by the platform implementing
>>> power domain enable/disable low level handlers according to the above
>>> defined notification types. This approach will considerably reduce the
>>> number of call to notifiers as compared to calling always from core
>>> powerdomain subsystem.
>>>
>>> Also the registered domain's will be managed inside a cache list and not
>>> part of the genpd structure. This will help in registration of notifiers from
>>> subsystems (such as clock) even when the PD subsystem is still not initialised.
>>> This list also filters out the unregistered pd's requesting notification.
>>>
>>> Cc: Rafael J. Wysocki <[e-mail]>
>>> Reviewed-by: Pankaj Dubey <[e-mail]>
>>> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
>>> ---
>>>  drivers/base/power/domain.c |  112 ++++++++++++++++++++++++++++++++++++++++++-
>>>  include/linux/pm_domain.h   |   31 ++++++++++++
>>>  2 files changed, 142 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 40bc2f4..e05045e 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -46,10 +46,19 @@
>>>       __retval;                                                               \
>>>  })
>>>
>>> +struct cache_notify_domains {
>>> +     char *name;
>>> +     /* Node in the cache pm domain name list */
>>> +     struct list_head cache_list_node;
>>> +};
>>> +
>>>  static LIST_HEAD(gpd_list);
>>>  static DEFINE_MUTEX(gpd_list_lock);
>>> +static LIST_HEAD(domain_notify_list);
>>> +static DEFINE_MUTEX(domain_notify_list_lock);
>>> +static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
>>>
>>> -static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>>  {
>>>       struct generic_pm_domain *genpd = NULL, *gpd;
>>>
>>> @@ -66,6 +75,7 @@ static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>>       mutex_unlock(&gpd_list_lock);
>>>       return genpd;
>>>  }
>>> +EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
>>>
>>>  struct generic_pm_domain *dev_to_genpd(struct device *dev)
>>>  {
>>> @@ -1908,6 +1918,106 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>>       mutex_unlock(&gpd_list_lock);
>>>  }
>>>
>>> +/**
>>> + * genpd_register_notifier - Register a PM domain for future notification.
>>> + * @nb: notification block containing notification handle.
>>> + * @pd_name: PM domain name.
>>> + */
>>> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
>>> +{
>>> +     int ret;
>>> +     struct cache_notify_domains *entry;
>>> +
>>> +     if (!pd_name)
>>> +             return -EINVAL;
>>> +
>>> +     /* Search if the pd is already registered */
>>> +     mutex_lock(&domain_notify_list_lock);
>>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>>> +             if (!strcmp(entry->name, pd_name))
>>> +                     break;
>>> +     }
>>> +     mutex_unlock(&domain_notify_list_lock);
>>> +
>>> +     if (entry) {
>>> +             pr_err("%s: pd already exists\n", __func__);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>>> +     if (!entry)
>>> +             return -ENOMEM;
>>> +
>>> +     entry->name = pd_name;
>>> +
>>> +     mutex_lock(&domain_notify_list_lock);
>>> +     list_add(&entry->cache_list_node, &domain_notify_list);
>>> +     mutex_unlock(&domain_notify_list_lock);
>>> +     ret = blocking_notifier_chain_register(
>>> +                             &genpd_transition_notifier_list, nb);
>>> +     return ret;
>>> +}
>>> +
>>> +/**
>>> + * genpd_unregister_notifier - Un-register a PM domain for future notification.
>>> + * @nb: notification block containing notification handle.
>>> + * @pd_name: PM domain name.
>>> + */
>>> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
>>> +{
>>> +     int ret;
>>> +     struct cache_notify_domains *entry;
>>> +
>>> +     mutex_lock(&domain_notify_list_lock);
>>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>>> +             if (!strcmp(entry->name, pd_name))
>>> +                     break;
>>> +     }
>>> +     if (!entry) {
>>> +             mutex_unlock(&domain_notify_list_lock);
>>> +             pr_err("%s: Invalid pd name\n", __func__);
>>> +             return -EINVAL;
>>> +     }
>>> +     list_del(&entry->cache_list_node);
>>> +     mutex_unlock(&domain_notify_list_lock);
>>> +     ret = blocking_notifier_chain_unregister(
>>> +                             &genpd_transition_notifier_list, nb);
>>> +     return ret;
>>> +}
>>> +
>>> +/**
>>> + * genpd_invoke_transition_notifier - Calls the matching notification handler.
>>> + * @genpd: generic power domain structure.
>>> + * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
>>> + */
>>> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>> +                     enum gpd_on_off_state state)
>>> +{
>>> +     struct cache_notify_domains *entry;
>>> +
>>> +     if (!genpd) {
>>> +             pr_err("Invalid genpd parameter\n");
>>> +             return;
>>> +     }
>>> +
>>> +     if (state != GPD_OFF_PRE || state != GPD_OFF_POST
>>> +                     || state != GPD_ON_PRE || state != GPD_ON_POST) {
>>> +             pr_err("Invalid state parameter\n");
>>> +             return;
>>> +     }
>>> +
>>> +     mutex_lock(&domain_notify_list_lock);
>>> +     list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
>>> +             if (!strcmp(entry->name, genpd->name))
>>> +                     break;
>>> +     }
>>> +     mutex_unlock(&domain_notify_list_lock);
>>> +     if (!entry) /* Simply ignore */
>>> +             return;
>>> +
>>> +     blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
>>> +                             genpd);
>>> +}
>>>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>>>  /*
>>>   * Device Tree based PM domain providers.
>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>>> index 73e938b..659997f 100644
>>> --- a/include/linux/pm_domain.h
>>> +++ b/include/linux/pm_domain.h
>>> @@ -25,6 +25,13 @@ enum gpd_status {
>>>       GPD_STATE_POWER_OFF,    /* PM domain is off */
>>>  };
>>>
>>> +enum gpd_on_off_state {
>>> +     GPD_OFF_PRE = 0,        /* GPD state before power off */
>>> +     GPD_OFF_POST,           /* GPD state after power off */
>>> +     GPD_ON_PRE,             /* GPD state before power on */
>>> +     GPD_ON_POST,            /* GPD state after power on */
>>> +};
>>> +
>>>  struct dev_power_governor {
>>>       bool (*power_down_ok)(struct dev_pm_domain *domain);
>>>       bool (*stop_ok)(struct device *dev);
>>> @@ -148,6 +155,12 @@ extern int pm_genpd_name_poweron(const char *domain_name);
>>>
>>>  extern struct dev_power_governor simple_qos_governor;
>>>  extern struct dev_power_governor pm_domain_always_on_gov;
>>> +
>>> +struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name);
>>> +int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>>> +int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>>> +void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>> +                     enum gpd_on_off_state state);
>>>  #else
>>>
>>>  static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>>> @@ -219,6 +232,24 @@ static inline int pm_genpd_name_poweron(const char *domain_name)
>>>  {
>>>       return -ENOSYS;
>>>  }
>>> +static inline struct
>>> +generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>> +{
>>> +     return NULL;
>>> +}
>>> +static inline int
>>> +genpd_register_notifier(struct notifier_block *nb, char *pd_name)
>>> +{
>>> +     return 0;
>>> +}
>>> +static inline int
>>> +genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
>>> +{
>>> +     return 0;
>>> +}
>>> +static inline void
>>> +genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>> +                     enum gpd_on_off_state state) { }
>>>  #define simple_qos_governor NULL
>>>  #define pm_domain_always_on_gov NULL
>>>  #endif
>>>
>>
>> --
>> I speak only for myself.
>> Rafael J. Wysocki, Intel Open Source Technology Center.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amit Kachhap Nov. 4, 2014, 6:44 a.m. UTC | #9

On Mon, Nov 3, 2014 at 11:53 PM, Kevin Hilman <[e-mail]> wrote:
> +Mike Turquette
>
> Hi Amit,
>
> "Rafael J. Wysocki" <[e-mail]> writes:
>
>> CC: Kevin, Ulf, Geert.
>>
>> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>>> These power domain transition notifiers will assist in carrying
>>> out some activity associated with domain power on/off such as
>>> some registers which may lose its contents and need save/restore
>>> across domain power off/on.
>
> The runtime PM framework already provides callbacks that are useful for
> context save/restore for devices.  Could you please describe in more
> detail which registers in which kind of devices need to be
> saved/restored, and why they cannot be saved/restored using existing
> mechanisms.
Basically the requirement is mandated by exynos7 manual. It tells that
before turning off the power domain, some clock registers need to saved
and should be restored just after turning the power domain. These clock
registers are not necessarily gate clocks but could be mux clocks etc.
The driver may not have all information of these clocks also. I suppose
these are Soc specific changes but drivers should work across Socs.
This behavior is almost similar to suspend/resume case where a whole
list of clock registers are saved/restored.

Even earlier post by Sylwester (https://lkml.org/lkml/2014/8/5/182) also points
to the need of this feature.
>
> Personally, I'm uncomfortable with notifiers like this because it
> suggests that underlying frameworks are not doing the right thing, or
> are not being used.  (I also don't like the implementation here where a
> single global notifier list is maintained by the core, but the notifiers
> are actually triggered by SoC specific code.)
Yes right the global notifier block can be moved to per genpd structure.
Also SoC trigger can be moved to core files.
>
> IIUC, the usage in this series seems to be that certain clock related
> registers need to be saved/restored across a power domain transition.
>
> Wouldn't an alternative solution be to add a feature to the clock driver
> such that the state of each clock is saved when the clock is disabled,
> and restored when the clock is enabled?   That would allow any clock
> context to survive any power domain transtion also, correct?
I also thought about same. But the trigger point for this would be
driver calling
clk disable/enable and not the power domain. so this will lead to lot of
save/restore for each power domain child.
>
> I have some issues with the implementaion as well, but I think we need
> to first sort out the real need for this before going into those
> details.
right :( may be I should have posted this as RFC
Thanks for the review.

Regards,
Amit D
>
> Kevin
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 4, 2014, 11:01 a.m. UTC | #10

On 04/11/14 07:44, amit daniel kachhap wrote:
> On Mon, Nov 3, 2014 at 11:53 PM, Kevin Hilman <[e-mail]> wrote:
>> "Rafael J. Wysocki" <[e-mail]> writes:
>>> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>>>> These power domain transition notifiers will assist in carrying
>>>> out some activity associated with domain power on/off such as
>>>> some registers which may lose its contents and need save/restore
>>>> across domain power off/on.
>>
>> The runtime PM framework already provides callbacks that are useful for
>> context save/restore for devices.  Could you please describe in more
>> detail which registers in which kind of devices need to be
>> saved/restored, and why they cannot be saved/restored using existing
>> mechanisms.
>
> Basically the requirement is mandated by exynos7 manual. It tells that
> before turning off the power domain, some clock registers need to saved
> and should be restored just after turning the power domain. These clock
> registers are not necessarily gate clocks but could be mux clocks etc.
> The driver may not have all information of these clocks also. I suppose
> these are Soc specific changes but drivers should work across Socs.
> This behavior is almost similar to suspend/resume case where a whole
> list of clock registers are saved/restored.

Indeed, the somehow complicated power domain power on/off sequences
are SoC specific.  They involve not only groups of clocks (usually
gate, mux clock registers of all devices in a power domain) but also
SoC-specific PMU (Power Management Unit) registers.
I assume it would be inappropriate to push such details to device 
drivers.  Moreover, a device driver could not be even loaded.

Since the clocks' state is already maintained by clk driver we came 
up with an idea of having generic calls from power domain driver back 
to the clock controller driver.

It might not to be the prettiest solution, nevertheless I couldn't come 
up with a better one which would satisfy all the requirements.  The power
domain should only be provided for use with all the clk/PMU sequences 
handling in place.

Clocks need to also be touched before a power domain switch on or off,
so attaching the clock controller to some power domain wouldn't help,
unless we modify/add to existing power domain related callbacks for 
devices.  Another issue is the clock controller device would need to 
be attached to multiple power domains, and for each power domain the 
power on/off sequence is usually slightly different.

There are also hierarchical power domains where each: the master and
the sub-domain need they own sequence and device usually is attached
only to a sub-domain.  

> Even earlier post by Sylwester (https://lkml.org/lkml/2014/8/5/182) 
> also points to the need of this feature.
>>
>> Personally, I'm uncomfortable with notifiers like this because it
>> suggests that underlying frameworks are not doing the right thing, or
>> are not being used.  (I also don't like the implementation here where a
>> single global notifier list is maintained by the core, but the notifiers
>> are actually triggered by SoC specific code.)
>
> Yes right the global notifier block can be moved to per genpd structure.
> Also SoC trigger can be moved to core files.
>>
>> IIUC, the usage in this series seems to be that certain clock related
>> registers need to be saved/restored across a power domain transition.
>>
>> Wouldn't an alternative solution be to add a feature to the clock driver
>> such that the state of each clock is saved when the clock is disabled,
>> and restored when the clock is enabled?   That would allow any clock
>> context to survive any power domain transtion also, correct?
>
> I also thought about same. But the trigger point for this would be
> driver calling clk disable/enable and not the power domain. so this 
> will lead to lot of save/restore for each power domain child.

Even though we would have saved/restored at that points still the power
domain driver would need to enforce some specific clock/PMU registers 
state before/after a power domain state transition. And this is what I 
found difficult with the existing APIs.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 4, 2014, 12:08 p.m. UTC | #11

On 04/11/14 07:16, amit daniel kachhap wrote:
> On Mon, Nov 3, 2014 at 11:51 PM, Sylwester Nawrocki
> <[e-mail]> wrote:
>> On 03/11/14 04:53, Amit Daniel Kachhap wrote:
[...]
>>> 4 type of notifications are added,
>>> GPD_OFF_PRE     - GPD state before power off
>>> GPD_OFF_POST    - GPD state after power off
>>> GPD_ON_PRE      - GPD state before power off
>>> GPD_ON_POST     - GPD state after power off
>>>
>>> 3 notfication API's are exported.
>>> 1) int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
>>> 2) int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
>>> 3) void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
>>>                       enum gpd_on_off_state state);
>>>
>>> In this approach the notifiers are registered/unregistered with pd name.
>>> The actual invoking of the notfiers will be done by the platform implementing
>>> power domain enable/disable low level handlers according to the above
>>> defined notification types. This approach will considerably reduce the
>>> number of call to notifiers as compared to calling always from core
>>> powerdomain subsystem.
>>>
>>> Also the registered domain's will be managed inside a cache list and not
>>> part of the genpd structure. This will help in registration of notifiers from
>>> subsystems (such as clock) even when the PD subsystem is still not initialised.
>>> This list also filters out the unregistered pd's requesting notification.
>>
>> This patch is somewhat similar my patch adding power domain state change
>> notifications [1].  I have already a reworked version of that patch, with the
>> notifier list moved to struct generic_pm_domain and using genpd->lock instead
> 
> Yes this will be correct as others also suggested to make per genpd
> notifier block.
> 
>> of dev->power.lock.  Anyway, while I'd leave the decision about the location
>> from where the notifier chains are supposed to be called to the subsystem's
>> maintainers preference, I'm rather reluctant to having one global notifiers
>> list for all possible power domains and all the notification clients.
>> The possibly long list needs to be traversed at each notifier call and it
>> seems in your implementation any registered user of the notification gets
>> notifications for all possible power domains.
>>
>> [1] https://lkml.org/lkml/2014/8/5/182
>
> My fault, I somehow missed this link earlier. After going through
> this, I found it registers genpd from the platform driver, so the
> function signature is
> int pm_genpd_register_notifier(struct device *dev, struct notifier_block *nb);
> I suggest to make the function signature to be like,
> int pm_genpd_register_notifier(struct device_node *np, struct
> notifier_block *nb)
> 
> In this way this function should should be able to support both
> platform devices and non platform devices like clk.
> 
> The function may work like,
> 
> pdev = of_find_device_by_node(np);
> if (pdev) {
>          //  get genpd from device and go ahead with notfier registration.
>          // blocking_notifier_chain_register(genpd->pd_notifier, nb)
> } else
> {
>         // get pd_handle from np
>         // get the pd_name from phandle and try registering this gen pd
>         // if the genpd subsystem is not initialised then add this in
> a temporary list and register the notifier later
> }
> 
> Can you post your implementation with 1st part ? Later I can post the
> else part with my changes.

Hmm, I doubt making the first argument of the notifier registration
function 'struct device_node *' is a good idea. AFAIK we shouldn't be
introducing and requiring device tree in generic interfaces.

We already tried an approach with pending notifier registration list.
I.e. when the power domain is not yet available we put the notification
registration request onto a separate list, which is then traversed
right before the power domain is registered with PM core.
Then we had a separate notification type for the notification client,
to let it know if his registration request has finally failed
permanently (invoked at some late initcall).

It all works, but it's not something we're really happy with. I hope
we can work out some reasonable solution for mainline. I will post
my PD transition notification patch, however it's mostly to demonstrate
the problem. In v2 PD can be also referred to by name, beside allowing
to just pass a struct device * to a device which belongs to a power
domain.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Nov. 7, 2014, 6:45 p.m. UTC | #12

Sylwester Nawrocki <[e-mail]> writes:

> On 04/11/14 07:44, amit daniel kachhap wrote:
>> On Mon, Nov 3, 2014 at 11:53 PM, Kevin Hilman <[e-mail]> wrote:
>>> "Rafael J. Wysocki" <[e-mail]> writes:
>>>> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>>>>> These power domain transition notifiers will assist in carrying
>>>>> out some activity associated with domain power on/off such as
>>>>> some registers which may lose its contents and need save/restore
>>>>> across domain power off/on.
>>>
>>> The runtime PM framework already provides callbacks that are useful for
>>> context save/restore for devices.  Could you please describe in more
>>> detail which registers in which kind of devices need to be
>>> saved/restored, and why they cannot be saved/restored using existing
>>> mechanisms.
>>
>> Basically the requirement is mandated by exynos7 manual. It tells that
>> before turning off the power domain, some clock registers need to saved
>> and should be restored just after turning the power domain. These clock
>> registers are not necessarily gate clocks but could be mux clocks etc.
>> The driver may not have all information of these clocks also. I suppose
>> these are Soc specific changes but drivers should work across Socs.
>> This behavior is almost similar to suspend/resume case where a whole
>> list of clock registers are saved/restored.
>
> Indeed, the somehow complicated power domain power on/off sequences
> are SoC specific.  They involve not only groups of clocks (usually
> gate, mux clock registers of all devices in a power domain) but also
> SoC-specific PMU (Power Management Unit) registers.
> I assume it would be inappropriate to push such details to device 
> drivers.  Moreover, a device driver could not be even loaded.
>
> Since the clocks' state is already maintained by clk driver we came 
> up with an idea of having generic calls from power domain driver back 
> to the clock controller driver.

For the clock tree, it still seems to me that this is better handled in
the SoC clock driver.  For example, when a power domain is about to be
gated, all the devices in that domain are runtime suspended, and
presumably all of their gate clocks are disabled.  Now, doesn't the
clock driver know the clock tree parent-child hierarchy and shouldn't it
be capable of saving the state of parent clocks (like mux clocks) etc?

Stated diffrently, it still seems to me like we're pushing functionality
in PM core notifiers that should be the responsibility of subsystem
drivers. 

> It might not to be the prettiest solution, nevertheless I couldn't come 
> up with a better one which would satisfy all the requirements.  The power
> domain should only be provided for use with all the clk/PMU sequences 
> handling in place.
>
> Clocks need to also be touched before a power domain switch on or off,
> so attaching the clock controller to some power domain wouldn't help,
> unless we modify/add to existing power domain related callbacks for 
> devices.  Another issue is the clock controller device would need to 
> be attached to multiple power domains, and for each power domain the 
> power on/off sequence is usually slightly different.
>
> There are also hierarchical power domains where each: the master and
> the sub-domain need they own sequence and device usually is attached
> only to a sub-domain.  
>
>> Even earlier post by Sylwester (https://lkml.org/lkml/2014/8/5/182) 
>> also points to the need of this feature.
>>>
>>> Personally, I'm uncomfortable with notifiers like this because it
>>> suggests that underlying frameworks are not doing the right thing, or
>>> are not being used.  (I also don't like the implementation here where a
>>> single global notifier list is maintained by the core, but the notifiers
>>> are actually triggered by SoC specific code.)
>>
>> Yes right the global notifier block can be moved to per genpd structure.
>> Also SoC trigger can be moved to core files.
>>>
>>> IIUC, the usage in this series seems to be that certain clock related
>>> registers need to be saved/restored across a power domain transition.
>>>
>>> Wouldn't an alternative solution be to add a feature to the clock driver
>>> such that the state of each clock is saved when the clock is disabled,
>>> and restored when the clock is enabled?   That would allow any clock
>>> context to survive any power domain transtion also, correct?
>>
>> I also thought about same. But the trigger point for this would be
>> driver calling clk disable/enable and not the power domain. so this 
>> will lead to lot of save/restore for each power domain child.
>
> Even though we would have saved/restored at that points still the power
> domain driver would need to enforce some specific clock/PMU registers 
> state before/after a power domain state transition. And this is what I 
> found difficult with the existing APIs.

This is what I'm not understanding.

Why can't the power domain driver's power_on/power_off callback just
call the PMU APIs and/or the clk_enable/_disable calls it needs?

Kevin



--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amit Kachhap Nov. 10, 2014, 9:08 a.m. UTC | #13

On Sat, Nov 8, 2014 at 12:15 AM, Kevin Hilman <[e-mail]> wrote:
> Sylwester Nawrocki <[e-mail]> writes:
>
>> On 04/11/14 07:44, amit daniel kachhap wrote:
>>> On Mon, Nov 3, 2014 at 11:53 PM, Kevin Hilman <[e-mail]> wrote:
>>>> "Rafael J. Wysocki" <[e-mail]> writes:
>>>>> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
>>>>>> These power domain transition notifiers will assist in carrying
>>>>>> out some activity associated with domain power on/off such as
>>>>>> some registers which may lose its contents and need save/restore
>>>>>> across domain power off/on.
>>>>
>>>> The runtime PM framework already provides callbacks that are useful for
>>>> context save/restore for devices.  Could you please describe in more
>>>> detail which registers in which kind of devices need to be
>>>> saved/restored, and why they cannot be saved/restored using existing
>>>> mechanisms.
>>>
>>> Basically the requirement is mandated by exynos7 manual. It tells that
>>> before turning off the power domain, some clock registers need to saved
>>> and should be restored just after turning the power domain. These clock
>>> registers are not necessarily gate clocks but could be mux clocks etc.
>>> The driver may not have all information of these clocks also. I suppose
>>> these are Soc specific changes but drivers should work across Socs.
>>> This behavior is almost similar to suspend/resume case where a whole
>>> list of clock registers are saved/restored.
>>
>> Indeed, the somehow complicated power domain power on/off sequences
>> are SoC specific.  They involve not only groups of clocks (usually
>> gate, mux clock registers of all devices in a power domain) but also
>> SoC-specific PMU (Power Management Unit) registers.
>> I assume it would be inappropriate to push such details to device
>> drivers.  Moreover, a device driver could not be even loaded.
>>
>> Since the clocks' state is already maintained by clk driver we came
>> up with an idea of having generic calls from power domain driver back
>> to the clock controller driver.
>
> For the clock tree, it still seems to me that this is better handled in
> the SoC clock driver.  For example, when a power domain is about to be
> gated, all the devices in that domain are runtime suspended, and
> presumably all of their gate clocks are disabled.  Now, doesn't the
> clock driver know the clock tree parent-child hierarchy and shouldn't it
> be capable of saving the state of parent clocks (like mux clocks) etc?
>
> Stated diffrently, it still seems to me like we're pushing functionality
> in PM core notifiers that should be the responsibility of subsystem
> drivers.
>
>> It might not to be the prettiest solution, nevertheless I couldn't come
>> up with a better one which would satisfy all the requirements.  The power
>> domain should only be provided for use with all the clk/PMU sequences
>> handling in place.
>>
>> Clocks need to also be touched before a power domain switch on or off,
>> so attaching the clock controller to some power domain wouldn't help,
>> unless we modify/add to existing power domain related callbacks for
>> devices.  Another issue is the clock controller device would need to
>> be attached to multiple power domains, and for each power domain the
>> power on/off sequence is usually slightly different.
>>
>> There are also hierarchical power domains where each: the master and
>> the sub-domain need they own sequence and device usually is attached
>> only to a sub-domain.
>>
>>> Even earlier post by Sylwester (https://lkml.org/lkml/2014/8/5/182)
>>> also points to the need of this feature.
>>>>
>>>> Personally, I'm uncomfortable with notifiers like this because it
>>>> suggests that underlying frameworks are not doing the right thing, or
>>>> are not being used.  (I also don't like the implementation here where a
>>>> single global notifier list is maintained by the core, but the notifiers
>>>> are actually triggered by SoC specific code.)
>>>
>>> Yes right the global notifier block can be moved to per genpd structure.
>>> Also SoC trigger can be moved to core files.
>>>>
>>>> IIUC, the usage in this series seems to be that certain clock related
>>>> registers need to be saved/restored across a power domain transition.
>>>>
>>>> Wouldn't an alternative solution be to add a feature to the clock driver
>>>> such that the state of each clock is saved when the clock is disabled,
>>>> and restored when the clock is enabled?   That would allow any clock
>>>> context to survive any power domain transtion also, correct?
>>>
>>> I also thought about same. But the trigger point for this would be
>>> driver calling clk disable/enable and not the power domain. so this
>>> will lead to lot of save/restore for each power domain child.
>>
>> Even though we would have saved/restored at that points still the power
>> domain driver would need to enforce some specific clock/PMU registers
>> state before/after a power domain state transition. And this is what I
>> found difficult with the existing APIs.
>
> This is what I'm not understanding.
>
> Why can't the power domain driver's power_on/power_off callback just
> call the PMU APIs and/or the clk_enable/_disable calls it needs?
This looks possible for clock enable/disable at least. Will further
check the implementation
feasibility and reply.

Regards,
Amit
>
> Kevin
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 28, 2014, 6:04 p.m. UTC | #14

On 07/11/14 19:45, Kevin Hilman wrote:
> Sylwester Nawrocki <[e-mail]> writes:
>> On 04/11/14 07:44, amit daniel kachhap wrote:
>>> On Mon, Nov 3, 2014 at 11:53 PM, Kevin Hilman <[e-mail]> wrote:
>>>> "Rafael J. Wysocki" <[e-mail]> writes:
>>>>> On Monday, November 03, 2014 09:23:01 AM Amit Daniel Kachhap wrote:
[...]
>> Indeed, the somehow complicated power domain power on/off sequences
>> are SoC specific.  They involve not only groups of clocks (usually
>> gate, mux clock registers of all devices in a power domain) but also
>> SoC-specific PMU (Power Management Unit) registers.
>> I assume it would be inappropriate to push such details to device 
>> drivers.  Moreover, a device driver could not be even loaded.
>>
>> Since the clocks' state is already maintained by clk driver we came 
>> up with an idea of having generic calls from power domain driver back 
>> to the clock controller driver.
> 
> For the clock tree, it still seems to me that this is better handled in
> the SoC clock driver.  For example, when a power domain is about to be
> gated, all the devices in that domain are runtime suspended, and
> presumably all of their gate clocks are disabled.  Now, doesn't the
> clock driver know the clock tree parent-child hierarchy and shouldn't it
> be capable of saving the state of parent clocks (like mux clocks) etc?
> 
> Stated diffrently, it still seems to me like we're pushing functionality
> in PM core notifiers that should be the responsibility of subsystem
> drivers. 

My apologies for not replying earlier, I got distracted by other 
activities.

I'd prefer not adding anything new to the PM core, however there are
dependencies between the power domain and clock controller driver 
which are hard to model in the current APIs.  I assume resorting to 
inter-exynos-driver API is not a good idea either.

Saving/restoring the clock hierarchy in the clock controller driver
during the power domain state transitions has a caveat that the clock
turn off/on sequences are IP/SoC subsystem specific.  So simply 
restoring saved registers from memory is not going to work.

The other detail I might have forgotten to mention is that the whole
clock controller may be in same power domain as the consumer devices.
That means the clock controller's registers must not be touched when
a related power domain is turned off.  Naturally when a power domain 
gets switched off all the clock controller's registers reset to their 
default values.  If we decided to use pm_runtime_{get_sync, put} in
the clock controller driver I'm not sure how it would need to interact
with the clk API.
In current mainline there is an issue with exynos4x12 that the system
may hang if clk_summary is attempted to read as the clock controller
driver doesn't take the ISP power domain into account.

[...]
>>>> Personally, I'm uncomfortable with notifiers like this because it
>>>> suggests that underlying frameworks are not doing the right thing, or
>>>> are not being used.  (I also don't like the implementation here where a
>>>> single global notifier list is maintained by the core, but the notifiers
>>>> are actually triggered by SoC specific code.)
>>>
>>> Yes right the global notifier block can be moved to per genpd structure.
>>> Also SoC trigger can be moved to core files.
>>>>
>>>> IIUC, the usage in this series seems to be that certain clock related
>>>> registers need to be saved/restored across a power domain transition.
>>>>
>>>> Wouldn't an alternative solution be to add a feature to the clock driver
>>>> such that the state of each clock is saved when the clock is disabled,
>>>> and restored when the clock is enabled?   That would allow any clock
>>>> context to survive any power domain transtion also, correct?
>>>
>>> I also thought about same. But the trigger point for this would be
>>> driver calling clk disable/enable and not the power domain. so this 
>>> will lead to lot of save/restore for each power domain child.
>>
>> Even though we would have saved/restored at that points still the power
>> domain driver would need to enforce some specific clock/PMU registers 
>> state before/after a power domain state transition. And this is what I 
>> found difficult with the existing APIs.
> 
> This is what I'm not understanding.
> 
> Why can't the power domain driver's power_on/power_off callback just
> call the PMU APIs and/or the clk_enable/_disable calls it needs?

I was concerned that it would not have been reliable by using the clk 
API due to the clk enable refcounting.  But that might not be a valid 
argument, since as you pointed out when a power domain is about to 
be gated related the clocks should be already disabled.

The other concern was atomicity in enabling/disabling groups of clocks,
i.e. setting group of bits in a clock gate register at once, rather
than a bit for each clock one by one.  But I'm not 100% sure about 
such a hardware requirement myself, would need to do some more testing 
and/or find a hardware engineer who could explain this. 

Additionally specifying clocks in device tree would be a bit messy and
there would very likely anyway be an additional information required 
in the power domain driver per each power domain regarding the clock 
handling sequence.

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 40bc2f4..e05045e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -46,10 +46,19 @@ 
 	__retval;								\
 })
 
+struct cache_notify_domains {
+	char *name;
+	/* Node in the cache pm domain name list */
+	struct list_head cache_list_node;
+};
+
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
+static LIST_HEAD(domain_notify_list);
+static DEFINE_MUTEX(domain_notify_list_lock);
+static BLOCKING_NOTIFIER_HEAD(genpd_transition_notifier_list);
 
-static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
+struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;
 
@@ -66,6 +75,7 @@  static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 	mutex_unlock(&gpd_list_lock);
 	return genpd;
 }
+EXPORT_SYMBOL_GPL(pm_genpd_lookup_name);
 
 struct generic_pm_domain *dev_to_genpd(struct device *dev)
 {
@@ -1908,6 +1918,106 @@  void pm_genpd_init(struct generic_pm_domain *genpd,
 	mutex_unlock(&gpd_list_lock);
 }
 
+/**
+ * genpd_register_notifier - Register a PM domain for future notification.
+ * @nb: notification block containing notification handle.
+ * @pd_name: PM domain name.
+ */
+int genpd_register_notifier(struct notifier_block *nb, char *pd_name)
+{
+	int ret;
+	struct cache_notify_domains *entry;
+
+	if (!pd_name)
+		return -EINVAL;
+
+	/* Search if the pd is already registered */
+	mutex_lock(&domain_notify_list_lock);
+	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
+		if (!strcmp(entry->name, pd_name))
+			break;
+	}
+	mutex_unlock(&domain_notify_list_lock);
+
+	if (entry) {
+		pr_err("%s: pd already exists\n", __func__);
+		return -EINVAL;
+	}
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	entry->name = pd_name;
+
+	mutex_lock(&domain_notify_list_lock);
+	list_add(&entry->cache_list_node, &domain_notify_list);
+	mutex_unlock(&domain_notify_list_lock);
+	ret = blocking_notifier_chain_register(
+				&genpd_transition_notifier_list, nb);
+	return ret;
+}
+
+/**
+ * genpd_unregister_notifier - Un-register a PM domain for future notification.
+ * @nb: notification block containing notification handle.
+ * @pd_name: PM domain name.
+ */
+int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
+{
+	int ret;
+	struct cache_notify_domains *entry;
+
+	mutex_lock(&domain_notify_list_lock);
+	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
+		if (!strcmp(entry->name, pd_name))
+			break;
+	}
+	if (!entry) {
+		mutex_unlock(&domain_notify_list_lock);
+		pr_err("%s: Invalid pd name\n", __func__);
+		return -EINVAL;
+	}
+	list_del(&entry->cache_list_node);
+	mutex_unlock(&domain_notify_list_lock);
+	ret = blocking_notifier_chain_unregister(
+				&genpd_transition_notifier_list, nb);
+	return ret;
+}
+
+/**
+ * genpd_invoke_transition_notifier - Calls the matching notification handler.
+ * @genpd: generic power domain structure.
+ * @state: can be of type - GPD_OFF_PRE/GPD_OFF_POST/GPD_ON_PRE/GPD_ON_POST.
+ */
+void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
+			enum gpd_on_off_state state)
+{
+	struct cache_notify_domains *entry;
+
+	if (!genpd) {
+		pr_err("Invalid genpd parameter\n");
+		return;
+	}
+
+	if (state != GPD_OFF_PRE || state != GPD_OFF_POST
+			|| state != GPD_ON_PRE || state != GPD_ON_POST) {
+		pr_err("Invalid state parameter\n");
+		return;
+	}
+
+	mutex_lock(&domain_notify_list_lock);
+	list_for_each_entry(entry, &domain_notify_list, cache_list_node) {
+		if (!strcmp(entry->name, genpd->name))
+			break;
+	}
+	mutex_unlock(&domain_notify_list_lock);
+	if (!entry) /* Simply ignore */
+		return;
+
+	blocking_notifier_call_chain(&genpd_transition_notifier_list, state,
+				genpd);
+}
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 73e938b..659997f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -25,6 +25,13 @@  enum gpd_status {
 	GPD_STATE_POWER_OFF,	/* PM domain is off */
 };
 
+enum gpd_on_off_state {
+	GPD_OFF_PRE = 0,	/* GPD state before power off */
+	GPD_OFF_POST,		/* GPD state after power off */
+	GPD_ON_PRE,		/* GPD state before power on */
+	GPD_ON_POST,		/* GPD state after power on */
+};
+
 struct dev_power_governor {
 	bool (*power_down_ok)(struct dev_pm_domain *domain);
 	bool (*stop_ok)(struct device *dev);
@@ -148,6 +155,12 @@  extern int pm_genpd_name_poweron(const char *domain_name);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
+
+struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name);
+int genpd_register_notifier(struct notifier_block *nb, char *pd_name);
+int genpd_unregister_notifier(struct notifier_block *nb, char *pd_name);
+void genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
+			enum gpd_on_off_state state);
 #else
 
 static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
@@ -219,6 +232,24 @@  static inline int pm_genpd_name_poweron(const char *domain_name)
 {
 	return -ENOSYS;
 }
+static inline struct
+generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
+{
+	return NULL;
+}
+static inline int
+genpd_register_notifier(struct notifier_block *nb, char *pd_name)
+{
+	return 0;
+}
+static inline int
+genpd_unregister_notifier(struct notifier_block *nb, char *pd_name)
+{
+	return 0;
+}
+static inline void
+genpd_invoke_transition_notifier(struct generic_pm_domain *genpd,
+			enum gpd_on_off_state state) { }
 #define simple_qos_governor NULL
 #define pm_domain_always_on_gov NULL
 #endif