[v3,1/2] power: domain: handle power supplies that need interrupts

Commit Message

Martin Kepplinger July 18, 2022, 9:03 p.m. UTC

If the power-domains' power-supply node (regulator) needs
interrupts to work, the current setup with noirq callbacks cannot
work; for example a pmic regulator on i2c, when suspending, usually already
times out during suspend_noirq:

[   41.024193] buck4: failed to disable: -ETIMEDOUT

So fix system suspend and resume for these power-domains by using the
"outer" suspend/resume callbacks instead. Tested on the imx8mq-librem5 board,
but by looking at the dts, this will fix imx8mq-evk and possibly other boards
too.

Possibly one can find more changes than suspend/resume for this case. They
can be added later when testing them: This is designed so that genpd
providers just say "this power-supply" needs interrupts - without implying
what exactly should be configured in genpd.

Initially system suspend problems had been discussed at
https://lore.kernel.org/linux-arm-kernel/[e-mail]/
which led to discussing the pmic that contains the regulators which
serve as power-domain power-supplies:
https://lore.kernel.org/linux-pm/[e-mail]/T/

Signed-off-by: Martin Kepplinger <[e-mail]>
---

revision history
----------------
v3: (thank you Ulf)
* move DT parsing to gpcv2 and create a genpd flag that gets set

v2: (thank you Krzysztof)
* rewrite: find possible regulators' interrupts property in parents
  instead of inventing a new property.
https://lore.kernel.org/linux-arm-kernel/[e-mail]/

v1: (initial idea)
https://lore.kernel.org/linux-arm-kernel/[e-mail]/T/#t


 drivers/base/power/domain.c | 19 +++++++++++++++++++
 drivers/soc/imx/gpcv2.c     |  9 +++++++++
 include/linux/pm_domain.h   |  6 ++++++
 3 files changed, 34 insertions(+)

Comments

Martin Kepplinger July 18, 2022, 9:05 p.m. UTC | #1

Am Montag, dem 18.07.2022 um 23:03 +0200 schrieb Martin Kepplinger:
> If the power-domains' power-supply node (regulator) needs
> interrupts to work, the current setup with noirq callbacks cannot
> work; for example a pmic regulator on i2c, when suspending, usually
> already
> times out during suspend_noirq:
> 
> [   41.024193] buck4: failed to disable: -ETIMEDOUT
> 
> So fix system suspend and resume for these power-domains by using the
> "outer" suspend/resume callbacks instead. Tested on the imx8mq-
> librem5 board,
> but by looking at the dts, this will fix imx8mq-evk and possibly
> other boards
> too.
> 
> Possibly one can find more changes than suspend/resume for this case.
> They
> can be added later when testing them: This is designed so that genpd
> providers just say "this power-supply" needs interrupts - without
> implying
> what exactly should be configured in genpd.
> 
> Initially system suspend problems had been discussed at
> https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> which led to discussing the pmic that contains the regulators which
> serve as power-domain power-supplies:
> https://lore.kernel.org/linux-pm/[e-mail]/T/
> 
> Signed-off-by: Martin Kepplinger <[e-mail]>
> ---
> 
> revision history
> ----------------
> v3: (thank you Ulf)
> * move DT parsing to gpcv2 and create a genpd flag that gets set
> 
> v2: (thank you Krzysztof)
> * rewrite: find possible regulators' interrupts property in parents
>   instead of inventing a new property.
> https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> 
> v1: (initial idea)
> https://lore.kernel.org/linux-arm-kernel/[e-mail]/T/#t
> 
> 
>  drivers/base/power/domain.c | 19 +++++++++++++++++++
>  drivers/soc/imx/gpcv2.c     |  9 +++++++++
>  include/linux/pm_domain.h   |  6 ++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c
> b/drivers/base/power/domain.c
> index 739e52cd4aba..1437476c9086 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -130,6 +130,7 @@ static const struct genpd_lock_ops genpd_spin_ops
> = {
>  #define genpd_is_active_wakeup(genpd)  (genpd->flags &
> GENPD_FLAG_ACTIVE_WAKEUP)
>  #define genpd_is_cpu_domain(genpd)     (genpd->flags &
> GENPD_FLAG_CPU_DOMAIN)
>  #define genpd_is_rpm_always_on(genpd)  (genpd->flags &
> GENPD_FLAG_RPM_ALWAYS_ON)
> +#define genpd_ps_needs_irq(genpd)      (genpd->flags &
> GENPD_FLAG_IRQ_POWER_SUPPLY)
>  
>  static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
>                 const struct generic_pm_domain *genpd)
> @@ -2298,6 +2299,20 @@ static bool genpd_present(const struct
> generic_pm_domain *genpd)
>         return ret;
>  }
>  
> +/**
> + * of_genpd_get_power_supply_irq() - Adjust if power-supply needs
> interrupts
> + * @genpd: Pointer to PM domain associated with the PM domain
> provider.
> + */
> +static void of_genpd_config_power_supply_irq(struct
> generic_pm_domain *pd)
> +{
> +       if (genpd_ps_needs_irq(pd)) {
> +               pd->domain.ops.suspend = genpd_suspend_noirq;
> +               pd->domain.ops.resume = genpd_resume_noirq;
> +               pd->domain.ops.suspend_noirq = NULL;
> +               pd->domain.ops.resume_noirq = NULL;
> +       }
> +}
> +
>  /**
>   * of_genpd_add_provider_simple() - Register a simple PM domain
> provider
>   * @np: Device node pointer associated with the PM domain provider.
> @@ -2343,6 +2358,8 @@ int of_genpd_add_provider_simple(struct
> device_node *np,
>         genpd->provider = &np->fwnode;
>         genpd->has_provider = true;
>  
> +       of_genpd_config_power_supply_irq(genpd);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> @@ -2394,6 +2411,8 @@ int of_genpd_add_provider_onecell(struct
> device_node *np,
>  
>                 genpd->provider = &np->fwnode;
>                 genpd->has_provider = true;
> +
> +               of_genpd_config_power_supply_irq(genpd);
>         }
>  
>         ret = genpd_add_provider(np, data->xlate, data);
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 85aa86e1338a..3a22bad07534 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -1303,6 +1303,7 @@ static const struct imx_pgc_domain_data
> imx8mn_pgc_domain_data = {
>  static int imx_pgc_domain_probe(struct platform_device *pdev)
>  {
>         struct imx_pgc_domain *domain = pdev->dev.platform_data;
> +       struct device_node *dn;
>         int ret;
>  
>         domain->dev = &pdev->dev;
> @@ -1333,6 +1334,14 @@ static int imx_pgc_domain_probe(struct
> platform_device *pdev)
>                 regmap_update_bits(domain->regmap, domain->regs->map,
>                                    domain->bits.map, domain-
> >bits.map);
>  
> +       dn = of_parse_phandle(domain->dev->of_node, "power-supply",
> 0);
> +       if (dn) {
> +               while ((dn = of_get_next_parent(dn))) {
> +                       if (of_get_property(dn, "interrupts", NULL))
> +                               domain->genpd.flags |=
> GENPD_FLAG_IRQ_POWER_SUPPLY;
> +               }
> +       }
> +
>         ret = pm_genpd_init(&domain->genpd, NULL, true);
>         if (ret) {
>                 dev_err(domain->dev, "Failed to init power
> domain\n");
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ebc351698090..bcceaf376f36 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -60,6 +60,11 @@
>   * GENPD_FLAG_MIN_RESIDENCY:   Enable the genpd governor to consider
> its
>   *                             components' next wakeup when
> determining the
>   *                             optimal idle state.
> + *
> + * GENPD_FLAG_IRQ_POWER_SUPPLY:        The power-domains' power-
> supply (regulator)
> + *                             needs interrupts to work. Adjust
> accordingly.
> + *                             Use the outer suspend/resume
> callbacks instead
> + *                             of noirq for example.
>   */
>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> @@ -68,6 +73,7 @@
>  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
>  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
>  #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
> +#define GENPD_FLAG_IRQ_POWER_SUPPLY (1U << 7)
>  
>  enum gpd_status {
>         GENPD_STATE_ON = 0,     /* PM domain is on */


I just noticed this should be 2 patches. I'll wait for feedback before
resending. thanks,

                            martin

Ulf Hansson July 19, 2022, 8:53 a.m. UTC | #2

On Mon, 18 Jul 2022 at 23:04, Martin Kepplinger
<[e-mail]> wrote:
>
> If the power-domains' power-supply node (regulator) needs
> interrupts to work, the current setup with noirq callbacks cannot
> work; for example a pmic regulator on i2c, when suspending, usually already
> times out during suspend_noirq:
>
> [   41.024193] buck4: failed to disable: -ETIMEDOUT
>
> So fix system suspend and resume for these power-domains by using the
> "outer" suspend/resume callbacks instead. Tested on the imx8mq-librem5 board,
> but by looking at the dts, this will fix imx8mq-evk and possibly other boards
> too.
>
> Possibly one can find more changes than suspend/resume for this case. They
> can be added later when testing them: This is designed so that genpd
> providers just say "this power-supply" needs interrupts - without implying
> what exactly should be configured in genpd.
>
> Initially system suspend problems had been discussed at
> https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> which led to discussing the pmic that contains the regulators which
> serve as power-domain power-supplies:
> https://lore.kernel.org/linux-pm/[e-mail]/T/
>
> Signed-off-by: Martin Kepplinger <[e-mail]>
> ---
>
> revision history
> ----------------
> v3: (thank you Ulf)
> * move DT parsing to gpcv2 and create a genpd flag that gets set
>
> v2: (thank you Krzysztof)
> * rewrite: find possible regulators' interrupts property in parents
>   instead of inventing a new property.
> https://lore.kernel.org/linux-arm-kernel/[e-mail]/
>
> v1: (initial idea)
> https://lore.kernel.org/linux-arm-kernel/[e-mail]/T/#t
>
>
>  drivers/base/power/domain.c | 19 +++++++++++++++++++
>  drivers/soc/imx/gpcv2.c     |  9 +++++++++
>  include/linux/pm_domain.h   |  6 ++++++
>  3 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 739e52cd4aba..1437476c9086 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -130,6 +130,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>  #define genpd_is_active_wakeup(genpd)  (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
>  #define genpd_is_cpu_domain(genpd)     (genpd->flags & GENPD_FLAG_CPU_DOMAIN)
>  #define genpd_is_rpm_always_on(genpd)  (genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
> +#define genpd_ps_needs_irq(genpd)      (genpd->flags & GENPD_FLAG_IRQ_POWER_SUPPLY)
>
>  static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
>                 const struct generic_pm_domain *genpd)
> @@ -2298,6 +2299,20 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
>         return ret;
>  }
>
> +/**
> + * of_genpd_get_power_supply_irq() - Adjust if power-supply needs interrupts
> + * @genpd: Pointer to PM domain associated with the PM domain provider.
> + */
> +static void of_genpd_config_power_supply_irq(struct generic_pm_domain *pd)

This isn't an "of" function. Moreover, I think we just skip the
function all together and have the code in pm_genpd_init() instead.

> +{
> +       if (genpd_ps_needs_irq(pd)) {
> +               pd->domain.ops.suspend = genpd_suspend_noirq;
> +               pd->domain.ops.resume = genpd_resume_noirq;
> +               pd->domain.ops.suspend_noirq = NULL;
> +               pd->domain.ops.resume_noirq = NULL;
> +       }
> +}
> +
>  /**
>   * of_genpd_add_provider_simple() - Register a simple PM domain provider
>   * @np: Device node pointer associated with the PM domain provider.
> @@ -2343,6 +2358,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
>         genpd->provider = &np->fwnode;
>         genpd->has_provider = true;
>
> +       of_genpd_config_power_supply_irq(genpd);

Drop this. As stated above, I think the code belongs in pm_genpd_init().

> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> @@ -2394,6 +2411,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>
>                 genpd->provider = &np->fwnode;
>                 genpd->has_provider = true;
> +
> +               of_genpd_config_power_supply_irq(genpd);

Ditto.

>         }
>
>         ret = genpd_add_provider(np, data->xlate, data);
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 85aa86e1338a..3a22bad07534 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -1303,6 +1303,7 @@ static const struct imx_pgc_domain_data imx8mn_pgc_domain_data = {
>  static int imx_pgc_domain_probe(struct platform_device *pdev)
>  {
>         struct imx_pgc_domain *domain = pdev->dev.platform_data;
> +       struct device_node *dn;
>         int ret;
>
>         domain->dev = &pdev->dev;
> @@ -1333,6 +1334,14 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>                 regmap_update_bits(domain->regmap, domain->regs->map,
>                                    domain->bits.map, domain->bits.map);
>
> +       dn = of_parse_phandle(domain->dev->of_node, "power-supply", 0);
> +       if (dn) {
> +               while ((dn = of_get_next_parent(dn))) {
> +                       if (of_get_property(dn, "interrupts", NULL))
> +                               domain->genpd.flags |= GENPD_FLAG_IRQ_POWER_SUPPLY;
> +               }
> +       }
> +
>         ret = pm_genpd_init(&domain->genpd, NULL, true);
>         if (ret) {
>                 dev_err(domain->dev, "Failed to init power domain\n");
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ebc351698090..bcceaf376f36 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -60,6 +60,11 @@
>   * GENPD_FLAG_MIN_RESIDENCY:   Enable the genpd governor to consider its
>   *                             components' next wakeup when determining the
>   *                             optimal idle state.
> + *
> + * GENPD_FLAG_IRQ_POWER_SUPPLY:        The power-domains' power-supply (regulator)
> + *                             needs interrupts to work. Adjust accordingly.
> + *                             Use the outer suspend/resume callbacks instead
> + *                             of noirq for example.

I prefer a more generic name. How about GENPD_FLAG_IRQ_ON.

For the description, I would rather state that the genpd needs irqs to
stay on to be able to manage power on/off. Or something along those
lines.

>   */
>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> @@ -68,6 +73,7 @@
>  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
>  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
>  #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
> +#define GENPD_FLAG_IRQ_POWER_SUPPLY (1U << 7)
>
>  enum gpd_status {
>         GENPD_STATE_ON = 0,     /* PM domain is on */
> --
> 2.30.2
>

BTW, a more generic question. If you move away from using the *noirq
callbacks to the other suspend/resume callbacks in genpd to solve this
problem, that requires all devices that is attached to the PM domain
(genpd) to also *not* be managed with the "late/early" or the "noirq"
callbacks too. In other case, we may power off the PM domain while
some devices may still rely on it to be on.

Are you sure that this is the case?

Kind regards
Uffe

Lucas Stach July 19, 2022, 9:06 a.m. UTC | #3

Am Dienstag, dem 19.07.2022 um 10:53 +0200 schrieb Ulf Hansson:
> On Mon, 18 Jul 2022 at 23:04, Martin Kepplinger
> <[e-mail]> wrote:
> > 
> > If the power-domains' power-supply node (regulator) needs
> > interrupts to work, the current setup with noirq callbacks cannot
> > work; for example a pmic regulator on i2c, when suspending, usually already
> > times out during suspend_noirq:
> > 
> > [   41.024193] buck4: failed to disable: -ETIMEDOUT
> > 
> > So fix system suspend and resume for these power-domains by using the
> > "outer" suspend/resume callbacks instead. Tested on the imx8mq-librem5 board,
> > but by looking at the dts, this will fix imx8mq-evk and possibly other boards
> > too.
> > 
> > Possibly one can find more changes than suspend/resume for this case. They
> > can be added later when testing them: This is designed so that genpd
> > providers just say "this power-supply" needs interrupts - without implying
> > what exactly should be configured in genpd.
> > 
> > Initially system suspend problems had been discussed at
> > https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> > which led to discussing the pmic that contains the regulators which
> > serve as power-domain power-supplies:
> > https://lore.kernel.org/linux-pm/[e-mail]/T/
> > 
> > Signed-off-by: Martin Kepplinger <[e-mail]>
> > ---
> > 
> > revision history
> > ----------------
> > v3: (thank you Ulf)
> > * move DT parsing to gpcv2 and create a genpd flag that gets set
> > 
> > v2: (thank you Krzysztof)
> > * rewrite: find possible regulators' interrupts property in parents
> >   instead of inventing a new property.
> > https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> > 
> > v1: (initial idea)
> > https://lore.kernel.org/linux-arm-kernel/[e-mail]/T/#t
> > 
> > 
> >  drivers/base/power/domain.c | 19 +++++++++++++++++++
> >  drivers/soc/imx/gpcv2.c     |  9 +++++++++
> >  include/linux/pm_domain.h   |  6 ++++++
> >  3 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 739e52cd4aba..1437476c9086 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -130,6 +130,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
> >  #define genpd_is_active_wakeup(genpd)  (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
> >  #define genpd_is_cpu_domain(genpd)     (genpd->flags & GENPD_FLAG_CPU_DOMAIN)
> >  #define genpd_is_rpm_always_on(genpd)  (genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
> > +#define genpd_ps_needs_irq(genpd)      (genpd->flags & GENPD_FLAG_IRQ_POWER_SUPPLY)
> > 
> >  static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
> >                 const struct generic_pm_domain *genpd)
> > @@ -2298,6 +2299,20 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
> >         return ret;
> >  }
> > 
> > +/**
> > + * of_genpd_get_power_supply_irq() - Adjust if power-supply needs interrupts
> > + * @genpd: Pointer to PM domain associated with the PM domain provider.
> > + */
> > +static void of_genpd_config_power_supply_irq(struct generic_pm_domain *pd)
> 
> This isn't an "of" function. Moreover, I think we just skip the
> function all together and have the code in pm_genpd_init() instead.
> 
> > +{
> > +       if (genpd_ps_needs_irq(pd)) {
> > +               pd->domain.ops.suspend = genpd_suspend_noirq;
> > +               pd->domain.ops.resume = genpd_resume_noirq;
> > +               pd->domain.ops.suspend_noirq = NULL;
> > +               pd->domain.ops.resume_noirq = NULL;
> > +       }
> > +}
> > +
> >  /**
> >   * of_genpd_add_provider_simple() - Register a simple PM domain provider
> >   * @np: Device node pointer associated with the PM domain provider.
> > @@ -2343,6 +2358,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
> >         genpd->provider = &np->fwnode;
> >         genpd->has_provider = true;
> > 
> > +       of_genpd_config_power_supply_irq(genpd);
> 
> Drop this. As stated above, I think the code belongs in pm_genpd_init().
> 
> > +
> >         return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> > @@ -2394,6 +2411,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> > 
> >                 genpd->provider = &np->fwnode;
> >                 genpd->has_provider = true;
> > +
> > +               of_genpd_config_power_supply_irq(genpd);
> 
> Ditto.
> 
> >         }
> > 
> >         ret = genpd_add_provider(np, data->xlate, data);
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 85aa86e1338a..3a22bad07534 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -1303,6 +1303,7 @@ static const struct imx_pgc_domain_data imx8mn_pgc_domain_data = {
> >  static int imx_pgc_domain_probe(struct platform_device *pdev)
> >  {
> >         struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > +       struct device_node *dn;
> >         int ret;
> > 
> >         domain->dev = &pdev->dev;
> > @@ -1333,6 +1334,14 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
> >                 regmap_update_bits(domain->regmap, domain->regs->map,
> >                                    domain->bits.map, domain->bits.map);
> > 
> > +       dn = of_parse_phandle(domain->dev->of_node, "power-supply", 0);
> > +       if (dn) {
> > +               while ((dn = of_get_next_parent(dn))) {
> > +                       if (of_get_property(dn, "interrupts", NULL))
> > +                               domain->genpd.flags |= GENPD_FLAG_IRQ_POWER_SUPPLY;
> > +               }
> > +       }
> > +
> >         ret = pm_genpd_init(&domain->genpd, NULL, true);
> >         if (ret) {
> >                 dev_err(domain->dev, "Failed to init power domain\n");
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index ebc351698090..bcceaf376f36 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -60,6 +60,11 @@
> >   * GENPD_FLAG_MIN_RESIDENCY:   Enable the genpd governor to consider its
> >   *                             components' next wakeup when determining the
> >   *                             optimal idle state.
> > + *
> > + * GENPD_FLAG_IRQ_POWER_SUPPLY:        The power-domains' power-supply (regulator)
> > + *                             needs interrupts to work. Adjust accordingly.
> > + *                             Use the outer suspend/resume callbacks instead
> > + *                             of noirq for example.
> 
> I prefer a more generic name. How about GENPD_FLAG_IRQ_ON.
> 
> For the description, I would rather state that the genpd needs irqs to
> stay on to be able to manage power on/off. Or something along those
> lines.
> 
> >   */
> >  #define GENPD_FLAG_PM_CLK       (1U << 0)
> >  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> > @@ -68,6 +73,7 @@
> >  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
> >  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
> >  #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
> > +#define GENPD_FLAG_IRQ_POWER_SUPPLY (1U << 7)
> > 
> >  enum gpd_status {
> >         GENPD_STATE_ON = 0,     /* PM domain is on */
> > --
> > 2.30.2
> > 
> 
> BTW, a more generic question. If you move away from using the *noirq
> callbacks to the other suspend/resume callbacks in genpd to solve this
> problem, that requires all devices that is attached to the PM domain
> (genpd) to also *not* be managed with the "late/early" or the "noirq"
> callbacks too. In other case, we may power off the PM domain while
> some devices may still rely on it to be on.
> 
> Are you sure that this is the case?

For the i.MX8M* it should be fine. While we have some devices that are
using the noirq supend/resume callbacks, like PCIe, those are not in a
power-domain where we would like to control an external regulator, so
things should work fine for the targeted use-case.

However, it may be a good idea to introduce some kind of kernel warning
when a driver with noirq suspend/resume callbacks is attached to a
GENPD_FLAG_IRQ_ON domain.

Regards,
Lucas

Ulf Hansson July 19, 2022, 9:52 a.m. UTC | #4

On Tue, 19 Jul 2022 at 11:06, Lucas Stach <[e-mail]> wrote:
>
> Am Dienstag, dem 19.07.2022 um 10:53 +0200 schrieb Ulf Hansson:
> > On Mon, 18 Jul 2022 at 23:04, Martin Kepplinger
> > <[e-mail]> wrote:
> > >
> > > If the power-domains' power-supply node (regulator) needs
> > > interrupts to work, the current setup with noirq callbacks cannot
> > > work; for example a pmic regulator on i2c, when suspending, usually already
> > > times out during suspend_noirq:
> > >
> > > [   41.024193] buck4: failed to disable: -ETIMEDOUT
> > >
> > > So fix system suspend and resume for these power-domains by using the
> > > "outer" suspend/resume callbacks instead. Tested on the imx8mq-librem5 board,
> > > but by looking at the dts, this will fix imx8mq-evk and possibly other boards
> > > too.
> > >
> > > Possibly one can find more changes than suspend/resume for this case. They
> > > can be added later when testing them: This is designed so that genpd
> > > providers just say "this power-supply" needs interrupts - without implying
> > > what exactly should be configured in genpd.
> > >
> > > Initially system suspend problems had been discussed at
> > > https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> > > which led to discussing the pmic that contains the regulators which
> > > serve as power-domain power-supplies:
> > > https://lore.kernel.org/linux-pm/[e-mail]/T/
> > >
> > > Signed-off-by: Martin Kepplinger <[e-mail]>
> > > ---
> > >
> > > revision history
> > > ----------------
> > > v3: (thank you Ulf)
> > > * move DT parsing to gpcv2 and create a genpd flag that gets set
> > >
> > > v2: (thank you Krzysztof)
> > > * rewrite: find possible regulators' interrupts property in parents
> > >   instead of inventing a new property.
> > > https://lore.kernel.org/linux-arm-kernel/[e-mail]/
> > >
> > > v1: (initial idea)
> > > https://lore.kernel.org/linux-arm-kernel/[e-mail]/T/#t
> > >
> > >
> > >  drivers/base/power/domain.c | 19 +++++++++++++++++++
> > >  drivers/soc/imx/gpcv2.c     |  9 +++++++++
> > >  include/linux/pm_domain.h   |  6 ++++++
> > >  3 files changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > index 739e52cd4aba..1437476c9086 100644
> > > --- a/drivers/base/power/domain.c
> > > +++ b/drivers/base/power/domain.c
> > > @@ -130,6 +130,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
> > >  #define genpd_is_active_wakeup(genpd)  (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
> > >  #define genpd_is_cpu_domain(genpd)     (genpd->flags & GENPD_FLAG_CPU_DOMAIN)
> > >  #define genpd_is_rpm_always_on(genpd)  (genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
> > > +#define genpd_ps_needs_irq(genpd)      (genpd->flags & GENPD_FLAG_IRQ_POWER_SUPPLY)
> > >
> > >  static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
> > >                 const struct generic_pm_domain *genpd)
> > > @@ -2298,6 +2299,20 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
> > >         return ret;
> > >  }
> > >
> > > +/**
> > > + * of_genpd_get_power_supply_irq() - Adjust if power-supply needs interrupts
> > > + * @genpd: Pointer to PM domain associated with the PM domain provider.
> > > + */
> > > +static void of_genpd_config_power_supply_irq(struct generic_pm_domain *pd)
> >
> > This isn't an "of" function. Moreover, I think we just skip the
> > function all together and have the code in pm_genpd_init() instead.
> >
> > > +{
> > > +       if (genpd_ps_needs_irq(pd)) {
> > > +               pd->domain.ops.suspend = genpd_suspend_noirq;
> > > +               pd->domain.ops.resume = genpd_resume_noirq;
> > > +               pd->domain.ops.suspend_noirq = NULL;
> > > +               pd->domain.ops.resume_noirq = NULL;
> > > +       }
> > > +}
> > > +
> > >  /**
> > >   * of_genpd_add_provider_simple() - Register a simple PM domain provider
> > >   * @np: Device node pointer associated with the PM domain provider.
> > > @@ -2343,6 +2358,8 @@ int of_genpd_add_provider_simple(struct device_node *np,
> > >         genpd->provider = &np->fwnode;
> > >         genpd->has_provider = true;
> > >
> > > +       of_genpd_config_power_supply_irq(genpd);
> >
> > Drop this. As stated above, I think the code belongs in pm_genpd_init().
> >
> > > +
> > >         return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> > > @@ -2394,6 +2411,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
> > >
> > >                 genpd->provider = &np->fwnode;
> > >                 genpd->has_provider = true;
> > > +
> > > +               of_genpd_config_power_supply_irq(genpd);
> >
> > Ditto.
> >
> > >         }
> > >
> > >         ret = genpd_add_provider(np, data->xlate, data);
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > index 85aa86e1338a..3a22bad07534 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -1303,6 +1303,7 @@ static const struct imx_pgc_domain_data imx8mn_pgc_domain_data = {
> > >  static int imx_pgc_domain_probe(struct platform_device *pdev)
> > >  {
> > >         struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > +       struct device_node *dn;
> > >         int ret;
> > >
> > >         domain->dev = &pdev->dev;
> > > @@ -1333,6 +1334,14 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
> > >                 regmap_update_bits(domain->regmap, domain->regs->map,
> > >                                    domain->bits.map, domain->bits.map);
> > >
> > > +       dn = of_parse_phandle(domain->dev->of_node, "power-supply", 0);
> > > +       if (dn) {
> > > +               while ((dn = of_get_next_parent(dn))) {
> > > +                       if (of_get_property(dn, "interrupts", NULL))
> > > +                               domain->genpd.flags |= GENPD_FLAG_IRQ_POWER_SUPPLY;
> > > +               }
> > > +       }
> > > +
> > >         ret = pm_genpd_init(&domain->genpd, NULL, true);
> > >         if (ret) {
> > >                 dev_err(domain->dev, "Failed to init power domain\n");
> > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > > index ebc351698090..bcceaf376f36 100644
> > > --- a/include/linux/pm_domain.h
> > > +++ b/include/linux/pm_domain.h
> > > @@ -60,6 +60,11 @@
> > >   * GENPD_FLAG_MIN_RESIDENCY:   Enable the genpd governor to consider its
> > >   *                             components' next wakeup when determining the
> > >   *                             optimal idle state.
> > > + *
> > > + * GENPD_FLAG_IRQ_POWER_SUPPLY:        The power-domains' power-supply (regulator)
> > > + *                             needs interrupts to work. Adjust accordingly.
> > > + *                             Use the outer suspend/resume callbacks instead
> > > + *                             of noirq for example.
> >
> > I prefer a more generic name. How about GENPD_FLAG_IRQ_ON.
> >
> > For the description, I would rather state that the genpd needs irqs to
> > stay on to be able to manage power on/off. Or something along those
> > lines.
> >
> > >   */
> > >  #define GENPD_FLAG_PM_CLK       (1U << 0)
> > >  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> > > @@ -68,6 +73,7 @@
> > >  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
> > >  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
> > >  #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
> > > +#define GENPD_FLAG_IRQ_POWER_SUPPLY (1U << 7)
> > >
> > >  enum gpd_status {
> > >         GENPD_STATE_ON = 0,     /* PM domain is on */
> > > --
> > > 2.30.2
> > >
> >
> > BTW, a more generic question. If you move away from using the *noirq
> > callbacks to the other suspend/resume callbacks in genpd to solve this
> > problem, that requires all devices that is attached to the PM domain
> > (genpd) to also *not* be managed with the "late/early" or the "noirq"
> > callbacks too. In other case, we may power off the PM domain while
> > some devices may still rely on it to be on.
> >
> > Are you sure that this is the case?
>
> For the i.MX8M* it should be fine. While we have some devices that are
> using the noirq supend/resume callbacks, like PCIe, those are not in a
> power-domain where we would like to control an external regulator, so
> things should work fine for the targeted use-case.

Alright, thanks for confirming!

>
> However, it may be a good idea to introduce some kind of kernel warning
> when a driver with noirq suspend/resume callbacks is attached to a
> GENPD_FLAG_IRQ_ON domain.

Yes, that sounds like a good idea.

>
> Regards,
> Lucas
>

Kind regards
Uffe

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 739e52cd4aba..1437476c9086 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -130,6 +130,7 @@  static const struct genpd_lock_ops genpd_spin_ops = {
 #define genpd_is_active_wakeup(genpd)	(genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
 #define genpd_is_cpu_domain(genpd)	(genpd->flags & GENPD_FLAG_CPU_DOMAIN)
 #define genpd_is_rpm_always_on(genpd)	(genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
+#define genpd_ps_needs_irq(genpd)	(genpd->flags & GENPD_FLAG_IRQ_POWER_SUPPLY)
 
 static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
 		const struct generic_pm_domain *genpd)
@@ -2298,6 +2299,20 @@  static bool genpd_present(const struct generic_pm_domain *genpd)
 	return ret;
 }
 
+/**
+ * of_genpd_get_power_supply_irq() - Adjust if power-supply needs interrupts
+ * @genpd: Pointer to PM domain associated with the PM domain provider.
+ */
+static void of_genpd_config_power_supply_irq(struct generic_pm_domain *pd)
+{
+	if (genpd_ps_needs_irq(pd)) {
+		pd->domain.ops.suspend = genpd_suspend_noirq;
+		pd->domain.ops.resume = genpd_resume_noirq;
+		pd->domain.ops.suspend_noirq = NULL;
+		pd->domain.ops.resume_noirq = NULL;
+	}
+}
+
 /**
  * of_genpd_add_provider_simple() - Register a simple PM domain provider
  * @np: Device node pointer associated with the PM domain provider.
@@ -2343,6 +2358,8 @@  int of_genpd_add_provider_simple(struct device_node *np,
 	genpd->provider = &np->fwnode;
 	genpd->has_provider = true;
 
+	of_genpd_config_power_supply_irq(genpd);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
@@ -2394,6 +2411,8 @@  int of_genpd_add_provider_onecell(struct device_node *np,
 
 		genpd->provider = &np->fwnode;
 		genpd->has_provider = true;
+
+		of_genpd_config_power_supply_irq(genpd);
 	}
 
 	ret = genpd_add_provider(np, data->xlate, data);
diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 85aa86e1338a..3a22bad07534 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -1303,6 +1303,7 @@  static const struct imx_pgc_domain_data imx8mn_pgc_domain_data = {
 static int imx_pgc_domain_probe(struct platform_device *pdev)
 {
 	struct imx_pgc_domain *domain = pdev->dev.platform_data;
+	struct device_node *dn;
 	int ret;
 
 	domain->dev = &pdev->dev;
@@ -1333,6 +1334,14 @@  static int imx_pgc_domain_probe(struct platform_device *pdev)
 		regmap_update_bits(domain->regmap, domain->regs->map,
 				   domain->bits.map, domain->bits.map);
 
+	dn = of_parse_phandle(domain->dev->of_node, "power-supply", 0);
+	if (dn) {
+		while ((dn = of_get_next_parent(dn))) {
+			if (of_get_property(dn, "interrupts", NULL))
+				domain->genpd.flags |= GENPD_FLAG_IRQ_POWER_SUPPLY;
+		}
+	}
+
 	ret = pm_genpd_init(&domain->genpd, NULL, true);
 	if (ret) {
 		dev_err(domain->dev, "Failed to init power domain\n");
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ebc351698090..bcceaf376f36 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -60,6 +60,11 @@ 
  * GENPD_FLAG_MIN_RESIDENCY:	Enable the genpd governor to consider its
  *				components' next wakeup when determining the
  *				optimal idle state.
+ *
+ * GENPD_FLAG_IRQ_POWER_SUPPLY:	The power-domains' power-supply (regulator)
+ *				needs interrupts to work. Adjust accordingly.
+ *				Use the outer suspend/resume callbacks instead
+ *				of noirq for example.
  */
 #define GENPD_FLAG_PM_CLK	 (1U << 0)
 #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
@@ -68,6 +73,7 @@ 
 #define GENPD_FLAG_CPU_DOMAIN	 (1U << 4)
 #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
 #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
+#define GENPD_FLAG_IRQ_POWER_SUPPLY (1U << 7)
 
 enum gpd_status {
 	GENPD_STATE_ON = 0,	/* PM domain is on */