From: Martin Kepplinger <martin.kepplinger@puri.sm> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: rafael@kernel.org, khilman@kernel.org, robh@kernel.org, krzysztof.kozlowski@linaro.org, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, pavel@ucw.cz, kernel@puri.sm, linux-imx@nxp.com, broonie@kernel.org, l.stach@pengutronix.de, aford173@gmail.com, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts Date: Thu, 25 Aug 2022 09:06:16 +0200 [thread overview] Message-ID: <3bbba64dc4fd9ef37fb937f5176b1ef50b8b2d73.camel@puri.sm> (raw) In-Reply-To: <CAPDyKFoS=E3c9XWWCaG2byMm-3nvvW5jXS0X7Bh-NK_msTUykQ@mail.gmail.com> Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > If for example 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 > > > > > > many other > > > > > > boards too. > > > > > > > > > > > > This is designed so that genpd providers just say "this > > > > > > genpd > > > > > > needs > > > > > > interrupts" (by setting the flag) - without implying an > > > > > > implementation. > > > > > > > > > > > > Initially system suspend problems had been discussed at > > > > > > https://lore.kernel.org/linux-arm-kernel/20211002005954.1367653-8-l.stach@pengutronix.de/ > > > > > > which led to discussing the pmic that contains the > > > > > > regulators > > > > > > which > > > > > > serve as power-domain power-supplies: > > > > > > https://lore.kernel.org/linux-pm/573166b75e524517782471c2b7f96e03fd93d175.camel@puri.sm/T/ > > > > > > > > > > > > Signed-off-by: Martin Kepplinger > > > > > > <martin.kepplinger@puri.sm> > > > > > > --- > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > b/drivers/base/power/domain.c > > > > > > index 5a2e0232862e..58376752a4de 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_irq_on(genpd) (genpd->flags & > > > > > > GENPD_FLAG_IRQ_ON) > > > > > > > > > > > > static inline bool irq_safe_dev_in_sleep_domain(struct > > > > > > device > > > > > > *dev, > > > > > > const struct generic_pm_domain *genpd) > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > generic_pm_domain > > > > > > *genpd, > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > genpd_runtime_suspend; > > > > > > genpd->domain.ops.runtime_resume = > > > > > > genpd_runtime_resume; > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > + > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > + genpd->domain.ops.suspend = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume = > > > > > > genpd_resume_noirq; > > > > > > + } else { > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > better > > > > > to > > > > > move to using genpd->domain.ops.suspend_late and > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > really > > > > want > > > > me to change the current behaviour (default case) to from noirq > > > > to > > > > late? Then I'll resend this series with such a patch added. > > > > > > Sorry, I wasn't clear enough, the default behaviour should remain > > > as > > > is. > > > > > > What I meant was, when genpd_irq_on() is true, we should use the > > > genpd->domain.ops.suspend_late and genpd- > > > >domain.ops.resume_early. > > > > Testing that shows that this isn't working. I can provide the logs > > later, but suspend fails and I think it makes sense: "suspend_late" > > is > > simply already too late when i2c (or any needed driver) uses > > "suspend". > > Okay, I see. > > The reason why I suggested moving the callbacks to "suspend_late", > was > that I was worried that some of the attached devices to genpd could > use "suspend_late" themselves. This is the case for some drivers for > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > curious to look at the DT files for the platform you are running, > would you mind giving me a pointer? I'm running https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi with these (small) patches on top: https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > So, this made me think about this a bit more. In the end, just using > different levels (suspend, suspend_late, suspend_noirq) of callbacks > are just papering over the real *dependency* problem. true, it doesn't feel like a stable solution. > > What we need for the genpd provider driver, is to be asked to be > suspended under the following conditions: > 1. All consumer devices (and child-domains) for its corresponding PM > domain have been suspended. > 2. All its supplier devices supplies must remain resumed, until the > genpd provider has been suspended. > > Please allow me a few more days to think in more detail about this. Thanks a lot for thinking about this! > > In some way, it looks like we should be able to combine the > information genpd has about its devices and child-domains, use PM > callbacks for the genpd provider driver - so we can rely on the > depency-path the fw_devlinks would give us for its supplier devices. > > Kind regards > Uffe martin
WARNING: multiple messages have this Message-ID (diff)
From: Martin Kepplinger <martin.kepplinger@puri.sm> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: rafael@kernel.org, khilman@kernel.org, robh@kernel.org, krzysztof.kozlowski@linaro.org, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, pavel@ucw.cz, kernel@puri.sm, linux-imx@nxp.com, broonie@kernel.org, l.stach@pengutronix.de, aford173@gmail.com, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts Date: Thu, 25 Aug 2022 09:06:16 +0200 [thread overview] Message-ID: <3bbba64dc4fd9ef37fb937f5176b1ef50b8b2d73.camel@puri.sm> (raw) In-Reply-To: <CAPDyKFoS=E3c9XWWCaG2byMm-3nvvW5jXS0X7Bh-NK_msTUykQ@mail.gmail.com> Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > If for example 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 > > > > > > many other > > > > > > boards too. > > > > > > > > > > > > This is designed so that genpd providers just say "this > > > > > > genpd > > > > > > needs > > > > > > interrupts" (by setting the flag) - without implying an > > > > > > implementation. > > > > > > > > > > > > Initially system suspend problems had been discussed at > > > > > > https://lore.kernel.org/linux-arm-kernel/20211002005954.1367653-8-l.stach@pengutronix.de/ > > > > > > which led to discussing the pmic that contains the > > > > > > regulators > > > > > > which > > > > > > serve as power-domain power-supplies: > > > > > > https://lore.kernel.org/linux-pm/573166b75e524517782471c2b7f96e03fd93d175.camel@puri.sm/T/ > > > > > > > > > > > > Signed-off-by: Martin Kepplinger > > > > > > <martin.kepplinger@puri.sm> > > > > > > --- > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > b/drivers/base/power/domain.c > > > > > > index 5a2e0232862e..58376752a4de 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_irq_on(genpd) (genpd->flags & > > > > > > GENPD_FLAG_IRQ_ON) > > > > > > > > > > > > static inline bool irq_safe_dev_in_sleep_domain(struct > > > > > > device > > > > > > *dev, > > > > > > const struct generic_pm_domain *genpd) > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > generic_pm_domain > > > > > > *genpd, > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > genpd_runtime_suspend; > > > > > > genpd->domain.ops.runtime_resume = > > > > > > genpd_runtime_resume; > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > + > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > + genpd->domain.ops.suspend = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume = > > > > > > genpd_resume_noirq; > > > > > > + } else { > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > better > > > > > to > > > > > move to using genpd->domain.ops.suspend_late and > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > really > > > > want > > > > me to change the current behaviour (default case) to from noirq > > > > to > > > > late? Then I'll resend this series with such a patch added. > > > > > > Sorry, I wasn't clear enough, the default behaviour should remain > > > as > > > is. > > > > > > What I meant was, when genpd_irq_on() is true, we should use the > > > genpd->domain.ops.suspend_late and genpd- > > > >domain.ops.resume_early. > > > > Testing that shows that this isn't working. I can provide the logs > > later, but suspend fails and I think it makes sense: "suspend_late" > > is > > simply already too late when i2c (or any needed driver) uses > > "suspend". > > Okay, I see. > > The reason why I suggested moving the callbacks to "suspend_late", > was > that I was worried that some of the attached devices to genpd could > use "suspend_late" themselves. This is the case for some drivers for > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > curious to look at the DT files for the platform you are running, > would you mind giving me a pointer? I'm running https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi with these (small) patches on top: https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > So, this made me think about this a bit more. In the end, just using > different levels (suspend, suspend_late, suspend_noirq) of callbacks > are just papering over the real *dependency* problem. true, it doesn't feel like a stable solution. > > What we need for the genpd provider driver, is to be asked to be > suspended under the following conditions: > 1. All consumer devices (and child-domains) for its corresponding PM > domain have been suspended. > 2. All its supplier devices supplies must remain resumed, until the > genpd provider has been suspended. > > Please allow me a few more days to think in more detail about this. Thanks a lot for thinking about this! > > In some way, it looks like we should be able to combine the > information genpd has about its devices and child-domains, use PM > callbacks for the genpd provider driver - so we can rely on the > depency-path the fw_devlinks would give us for its supplier devices. > > Kind regards > Uffe martin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-08-25 7:06 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-26 8:32 [PATCH v6 0/2] power: domain: handle power supplies that need interrupts Martin Kepplinger 2022-07-26 8:32 ` Martin Kepplinger 2022-07-26 8:32 ` [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts Martin Kepplinger 2022-07-26 8:32 ` Martin Kepplinger 2022-07-26 15:07 ` Ulf Hansson 2022-07-26 15:07 ` Ulf Hansson 2022-07-26 18:33 ` Rafael J. Wysocki 2022-07-26 18:33 ` Rafael J. Wysocki 2022-07-28 9:19 ` Ulf Hansson 2022-07-28 9:19 ` Ulf Hansson 2022-08-19 9:17 ` Martin Kepplinger 2022-08-19 9:17 ` Martin Kepplinger 2022-08-19 14:53 ` Ulf Hansson 2022-08-19 14:53 ` Ulf Hansson 2022-08-22 8:38 ` Martin Kepplinger 2022-08-22 8:38 ` Martin Kepplinger 2022-08-24 13:30 ` Ulf Hansson 2022-08-24 13:30 ` Ulf Hansson 2022-08-25 7:06 ` Martin Kepplinger [this message] 2022-08-25 7:06 ` Martin Kepplinger 2022-09-23 13:55 ` Ulf Hansson 2022-09-23 13:55 ` Ulf Hansson 2022-09-26 9:52 ` Martin Kepplinger 2022-09-26 9:52 ` Martin Kepplinger 2023-06-21 18:20 ` Martin Kepplinger 2023-06-21 18:20 ` Martin Kepplinger 2023-06-26 9:50 ` Ulf Hansson 2023-06-26 9:50 ` Ulf Hansson 2022-07-26 8:32 ` [PATCH v6 2/2] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON Martin Kepplinger 2022-07-26 8:32 ` Martin Kepplinger 2022-07-26 15:08 ` Ulf Hansson 2022-07-26 15:08 ` Ulf Hansson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3bbba64dc4fd9ef37fb937f5176b1ef50b8b2d73.camel@puri.sm \ --to=martin.kepplinger@puri.sm \ --cc=aford173@gmail.com \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=kernel@puri.sm \ --cc=khilman@kernel.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=l.stach@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=pavel@ucw.cz \ --cc=rafael@kernel.org \ --cc=robh@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ --cc=ulf.hansson@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.