Re: [RFC PATCH 2/3] pmdomain: core: Add a genpd config to support unknown initial status

On 26-08-19 17:14:52, Ulf Hansson wrote:
> From: Ulf Hansson <[email protected]>
> 
> It's not always possible for a genpd provider to know the initial status
> for its corresponding PM domain(s). To register the PM domain in a safe
> state, the genpd provider driver may therefore have to initialize the
> genpd's status to be powered off, as a way to prevent the PM domain from
> being used when it actually could be powered off.
> 
> In these cases we may end up to power on a PM domain through the genpd
> subsystem, while from the HW point of view it may already be powered on.
> Under these circumstances, it may also be required to keep the PM domain
> powered on until all the consumer devices have been probed. In other words,
> until the ->sync_state() callback for the genpd provider in question have
> been called.
> 
> To support this behaviour for a genpd provider, let's introduce a new genpd
> configuration, GENPD_FLAG_OFF_STAY_ON.
> 
> Reported-by: Maulik Shah <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
>  drivers/pmdomain/core.c   | 6 +++++-
>  include/linux/pm_domain.h | 9 +++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 7345c06f1d55..be4fff2f4664 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -189,6 +189,7 @@ static const struct genpd_lock_ops genpd_raw_spin_ops = {
>  #define genpd_is_dev_name_fw(genpd)	(genpd->flags & GENPD_FLAG_DEV_NAME_FW)
>  #define genpd_is_no_sync_state(genpd)	(genpd->flags & GENPD_FLAG_NO_SYNC_STATE)
>  #define genpd_is_no_stay_on(genpd)	(genpd->flags & GENPD_FLAG_NO_STAY_ON)
> +#define genpd_is_off_stay_on(genpd)	(genpd->flags & GENPD_FLAG_OFF_STAY_ON)

Maybe genpd_is_state_unknown ?

>  
>  static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
>  		const struct generic_pm_domain *genpd)
> @@ -2445,7 +2446,10 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  static void genpd_set_stay_on(struct generic_pm_domain *genpd, bool is_off)
>  {
> -	genpd->stay_on = !genpd_is_no_stay_on(genpd) && !is_off;
> +	if (genpd_is_off_stay_on(genpd))
> +		genpd->stay_on = is_off;
> +	else
> +		genpd->stay_on = !genpd_is_no_stay_on(genpd) && !is_off;
>  }
>  #else
>  static void genpd_set_stay_on(struct generic_pm_domain *genpd, bool is_off)
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f925614aebdb..9929347e1d7f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -121,6 +121,14 @@ struct dev_pm_domain_list {
>   *				powered-off until the ->sync_state() callback is
>   *				invoked. This flag informs genpd to allow a
>   *				power-off without waiting for ->sync_state().
> + *
> + * GENPD_FLAG_OFF_STAY_ON:	Use this flag to inform	genpd that its initial
> + *				status for the PM domain is set to powered off,
> + *				which may not correctly reflect the state of the
> + *				HW, as it's unknown. If the PM domain becomes
> + *				powered on during boot, genpd will prevent it
> + *				from being powered off until the ->sync_state
> + *				callback is invoked for it.

Maybe GENPD_FLAG_STATE_UNKNOWN ?

Or probably it would be better to just add a new state to the gpd_status
enum, like GENPD_STATE_UNKNOWN.

>   */
>  #define GENPD_FLAG_PM_CLK	 (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
> @@ -133,6 +141,7 @@ struct dev_pm_domain_list {
>  #define GENPD_FLAG_DEV_NAME_FW	 (1U << 8)
>  #define GENPD_FLAG_NO_SYNC_STATE (1U << 9)
>  #define GENPD_FLAG_NO_STAY_ON	 (1U << 10)
> +#define GENPD_FLAG_OFF_STAY_ON	 (1U << 11)
>  
>  enum gpd_status {
>  	GENPD_STATE_ON = 0,	/* PM domain is on */
> -- 
> 2.43.0
>