diff mbox series

[v5,08/13] ACPI: PM: Take wake IRQ into consideration when entering suspend-to-idle

Message ID 20220921094736.v5.8.I7d9202463f08373feccd6e8fd87482c4f40ece5d@changeid (mailing list archive)
State Superseded, archived
Headers show
Series acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq | expand

Commit Message

Raul Rangel Sept. 21, 2022, 3:52 p.m. UTC
This change adds support for ACPI devices that use ExclusiveAndWake or
SharedAndWake in their _CRS GpioInt definition (instead of using _PRW),
and also provide power resources. Previously the ACPI subsystem had no
idea if the device had a wake capable interrupt armed. This resulted
in the ACPI device PM system placing the device into D3Cold, and thus
cutting power to the device. With this change we will now query the
_S0W method to figure out the appropriate wake capable D-state.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

Changes in v5:
- Go back to using adev->wakeup.flags.valid to keep the diff cleaner
- Fix a typo in comment

 drivers/acpi/device_pm.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Rafael J. Wysocki Sept. 24, 2022, 5 p.m. UTC | #1
On Wed, Sep 21, 2022 at 5:52 PM Raul E Rangel <rrangel@chromium.org> wrote:
>
> This change adds support for ACPI devices that use ExclusiveAndWake or
> SharedAndWake in their _CRS GpioInt definition (instead of using _PRW),
> and also provide power resources. Previously the ACPI subsystem had no
> idea if the device had a wake capable interrupt armed. This resulted
> in the ACPI device PM system placing the device into D3Cold, and thus
> cutting power to the device. With this change we will now query the
> _S0W method to figure out the appropriate wake capable D-state.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
>
> Changes in v5:
> - Go back to using adev->wakeup.flags.valid to keep the diff cleaner
> - Fix a typo in comment
>
>  drivers/acpi/device_pm.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 9dce1245689ca2..3111fc426e04fd 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -681,8 +681,23 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>                 d_min = ret;
>                 wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
>                         && adev->wakeup.sleep_state >= target_state;
> -       } else {
> -               wakeup = adev->wakeup.flags.valid;
> +       } else if (adev->wakeup.flags.valid) {
> +               /* ACPI GPE specified in _PRW. */
> +               wakeup = true;

I would retain the "else" clause as it was and just add a new "else
if" one before it.

> +       } else if (device_may_wakeup(dev) && dev->power.wakeirq) {
> +               /*
> +                * The ACPI subsystem doesn't manage the wake bit for IRQs
> +                * defined with ExclusiveAndWake and SharedAndWake. Instead we
> +                * expect them to be managed via the PM subsystem. Drivers
> +                * should call dev_pm_set_wake_irq to register an IRQ as a wake
> +                * source.
> +                *
> +                * If a device has a wake IRQ attached we need to check the
> +                * _S0W method to get the correct wake D-state. Otherwise we
> +                * end up putting the device into D3Cold which will more than
> +                * likely disable wake functionality.
> +                */
> +               wakeup = true;
>         }
>
>         /*
> --
Raul Rangel Sept. 28, 2022, 9:10 p.m. UTC | #2
On Sat, Sep 24, 2022 at 11:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Sep 21, 2022 at 5:52 PM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > This change adds support for ACPI devices that use ExclusiveAndWake or
> > SharedAndWake in their _CRS GpioInt definition (instead of using _PRW),
> > and also provide power resources. Previously the ACPI subsystem had no
> > idea if the device had a wake capable interrupt armed. This resulted
> > in the ACPI device PM system placing the device into D3Cold, and thus
> > cutting power to the device. With this change we will now query the
> > _S0W method to figure out the appropriate wake capable D-state.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> > Changes in v5:
> > - Go back to using adev->wakeup.flags.valid to keep the diff cleaner
> > - Fix a typo in comment
> >
> >  drivers/acpi/device_pm.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index 9dce1245689ca2..3111fc426e04fd 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c

> > @@ -681,8 +681,23 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
> >                 d_min = ret;
> >                 wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
> >                         && adev->wakeup.sleep_state >= target_state;
Just an FYI, I didn't update the code that handles the target state >
S0. I need to get a
device that has S3 capabilities and the correct firmware to test this.
I figure I can do
that as a different patch when I have time to test with an S3 device.

> > -       } else {
> > -               wakeup = adev->wakeup.flags.valid;
> > +       } else if (adev->wakeup.flags.valid) {
> > +               /* ACPI GPE specified in _PRW. */
> > +               wakeup = true;

>
> I would retain the "else" clause as it was and just add a new "else
> if" one before it.
>
Done

> > +       } else if (device_may_wakeup(dev) && dev->power.wakeirq) {
> > +               /*
> > +                * The ACPI subsystem doesn't manage the wake bit for IRQs
> > +                * defined with ExclusiveAndWake and SharedAndWake. Instead we
> > +                * expect them to be managed via the PM subsystem. Drivers
> > +                * should call dev_pm_set_wake_irq to register an IRQ as a wake
> > +                * source.
> > +                *
> > +                * If a device has a wake IRQ attached we need to check the
> > +                * _S0W method to get the correct wake D-state. Otherwise we
> > +                * end up putting the device into D3Cold which will more than
> > +                * likely disable wake functionality.
> > +                */
> > +               wakeup = true;
> >         }
> >
> >         /*
> > --

I'll send out v6 soon unless anyone else has any comments.
Rafael J. Wysocki Sept. 29, 2022, 10:06 a.m. UTC | #3
On Wed, Sep 28, 2022 at 11:10 PM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Sat, Sep 24, 2022 at 11:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Sep 21, 2022 at 5:52 PM Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > This change adds support for ACPI devices that use ExclusiveAndWake or
> > > SharedAndWake in their _CRS GpioInt definition (instead of using _PRW),
> > > and also provide power resources. Previously the ACPI subsystem had no
> > > idea if the device had a wake capable interrupt armed. This resulted
> > > in the ACPI device PM system placing the device into D3Cold, and thus
> > > cutting power to the device. With this change we will now query the
> > > _S0W method to figure out the appropriate wake capable D-state.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > ---
> > >
> > > Changes in v5:
> > > - Go back to using adev->wakeup.flags.valid to keep the diff cleaner
> > > - Fix a typo in comment
> > >
> > >  drivers/acpi/device_pm.c | 19 +++++++++++++++++--
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > index 9dce1245689ca2..3111fc426e04fd 100644
> > > --- a/drivers/acpi/device_pm.c
> > > +++ b/drivers/acpi/device_pm.c
>
> > > @@ -681,8 +681,23 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
> > >                 d_min = ret;
> > >                 wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
> > >                         && adev->wakeup.sleep_state >= target_state;
> Just an FYI, I didn't update the code that handles the target state >
> S0. I need to get a
> device that has S3 capabilities and the correct firmware to test this.
> I figure I can do
> that as a different patch when I have time to test with an S3 device.

That's fine.

> > > -       } else {
> > > -               wakeup = adev->wakeup.flags.valid;
> > > +       } else if (adev->wakeup.flags.valid) {
> > > +               /* ACPI GPE specified in _PRW. */
> > > +               wakeup = true;
>
> >
> > I would retain the "else" clause as it was and just add a new "else
> > if" one before it.
> >
> Done

Thanks!

> > > +       } else if (device_may_wakeup(dev) && dev->power.wakeirq) {
> > > +               /*
> > > +                * The ACPI subsystem doesn't manage the wake bit for IRQs
> > > +                * defined with ExclusiveAndWake and SharedAndWake. Instead we
> > > +                * expect them to be managed via the PM subsystem. Drivers
> > > +                * should call dev_pm_set_wake_irq to register an IRQ as a wake
> > > +                * source.
> > > +                *
> > > +                * If a device has a wake IRQ attached we need to check the
> > > +                * _S0W method to get the correct wake D-state. Otherwise we
> > > +                * end up putting the device into D3Cold which will more than
> > > +                * likely disable wake functionality.
> > > +                */
> > > +               wakeup = true;
> > >         }
> > >
> > >         /*
> > > --
>
> I'll send out v6 soon unless anyone else has any comments.
diff mbox series

Patch

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 9dce1245689ca2..3111fc426e04fd 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -681,8 +681,23 @@  static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		d_min = ret;
 		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
 			&& adev->wakeup.sleep_state >= target_state;
-	} else {
-		wakeup = adev->wakeup.flags.valid;
+	} else if (adev->wakeup.flags.valid) {
+		/* ACPI GPE specified in _PRW. */
+		wakeup = true;
+	} else if (device_may_wakeup(dev) && dev->power.wakeirq) {
+		/*
+		 * The ACPI subsystem doesn't manage the wake bit for IRQs
+		 * defined with ExclusiveAndWake and SharedAndWake. Instead we
+		 * expect them to be managed via the PM subsystem. Drivers
+		 * should call dev_pm_set_wake_irq to register an IRQ as a wake
+		 * source.
+		 *
+		 * If a device has a wake IRQ attached we need to check the
+		 * _S0W method to get the correct wake D-state. Otherwise we
+		 * end up putting the device into D3Cold which will more than
+		 * likely disable wake functionality.
+		 */
+		wakeup = true;
 	}
 
 	/*