Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231108121940.288005-1-kai.heng.feng@canonical.com> <cc6c162407c69c53ec256bf887a0384361dd0516.camel@linux.intel.com>
In-Reply-To: <cc6c162407c69c53ec256bf887a0384361dd0516.camel@linux.intel.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Thu, 7 Dec 2023 10:44:47 +0800
Message-ID: <CAAd53p7do2rrB=qbpWKbNCWB_qfZ2YZPtB_55VcfGjDYXgLfzA@mail.gmail.com>
Subject: Re: [PATCH v2] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
To:     srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     jikos@kernel.org, benjamin.tissoires@redhat.com,
        linux-pm@vger.kernel.org, linux-pci@vger.kernel.org,
        Jian Hui Lee <jianhui.lee@canonical.com>,
        Even Xu <even.xu@intel.com>, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 5, 2023 at 1:50=E2=80=AFAM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Hi Kai,
>
> Sorry for he delay in getting back on this. I have a question below:
>
> On Wed, 2023-11-08 at 14:19 +0200, Kai-Heng Feng wrote:
> > Since PCI core and ACPI core already handles PCI PME wake and GPE
> > wake
> > when the device has wakeup capability, use device_init_wakeup() to
> > let
> > them do the wakeup setting work.
> >
> > Also add a shutdown callback which uses pci_prepare_to_sleep() to let
> > PCI and ACPI set OOB wakeup for S5.
> >
> > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v2:
> >  Rebase on ("HID: intel-ish-hid: ipc: Disable and reenable ACPI GPE
> > bit")
> >
> >  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 67 ++++++-----------------
> > --
> >  1 file changed, 15 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 710fda5f19e1..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,50 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> > pci_dev *pdev)
> >         return !pm_resume_via_firmware() || pdev->device =3D=3D
> > CHV_DEVICE_ID;
> >  }
> >
> > -static int enable_gpe(struct device *dev)
> > -{
> > -#ifdef CONFIG_ACPI
> > -       acpi_status acpi_sts;
> > -       struct acpi_device *adev;
> > -       struct acpi_device_wakeup *wakeup;
> > -
> > -       adev =3D ACPI_COMPANION(dev);
> > -       if (!adev) {
> > -               dev_err(dev, "get acpi handle failed\n");
> > -               return -ENODEV;
> > -       }
> > -       wakeup =3D &adev->wakeup;
> > -
> > -       /*
> > -        * Call acpi_disable_gpe(), so that reference count
> > -        * gpe_event_info->runtime_count doesn't overflow.
> > -        * When gpe_event_info->runtime_count =3D 0, the call
> > -        * to acpi_disable_gpe() simply return.
> > -        */
> > -       acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number);
> > -
> > -       acpi_sts =3D acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > >gpe_number);
> > -       if (ACPI_FAILURE(acpi_sts)) {
> > -               dev_err(dev, "enable ose_gpe failed\n");
> > -               return -EIO;
> > -       }
> > -
> > -       return 0;
> > -#else
> > -       return -ENODEV;
> > -#endif
> > -}
> > -
> > -static void enable_pme_wake(struct pci_dev *pdev)
> > -{
> > -       if ((pci_pme_capable(pdev, PCI_D0) ||
> > -            pci_pme_capable(pdev, PCI_D3hot) ||
> > -            pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > >dev)) {
> > -               pci_pme_active(pdev, true);
> > -               dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > enabled\n");
> > -       }
> > -}
> > -
> >  /**
> >   * ish_probe() - PCI driver probe callback
> >   * @pdev:      pci device
> > @@ -233,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> > struct pci_device_id *ent)
> >
> >         /* Enable PME for EHL */
> >         if (pdev->device =3D=3D EHL_Ax_DEVICE_ID)
> > -               enable_pme_wake(pdev);
> > +               device_init_wakeup(dev, true);
>
> For apple to apple comparison, which path will call
> https://elixir.bootlin.com/linux/latest/C/ident/__pci_enable_wake
> which will call pci_pme_active()?

Here's the flow:
device_shutdown()
  pci_device_shutdown()
    ish_shutdown()
      pci_prepare_to_sleep()
        __pci_enable_wake()
          pci_pme_active()
            __pci_pme_active()

Kai-Heng

>
> Thanks,
> Srinivas
>
> >
> >         ret =3D ish_init(ishtp);
> >         if (ret)
> > @@ -256,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> >         ish_device_disable(ishtp_dev);
> >  }
> >
> > +
> > +/**
> > + * ish_shutdown() - PCI driver shutdown callback
> > + * @pdev:      pci device
> > + *
> > + * This function sets up wakeup for S5
> > + */
> > +static void ish_shutdown(struct pci_dev *pdev)
> > +{
> > +       if (pdev->device =3D=3D EHL_Ax_DEVICE_ID)
> > +               pci_prepare_to_sleep(pdev);
> > +}
> > +
> >  static struct device __maybe_unused *ish_resume_device;
> >
> >  /* 50ms to get resume response */
> > @@ -378,13 +347,6 @@ static int __maybe_unused ish_resume(struct
> > device *device)
> >         struct pci_dev *pdev =3D to_pci_dev(device);
> >         struct ishtp_device *dev =3D pci_get_drvdata(pdev);
> >
> > -       /* add this to finish power flow for EHL */
> > -       if (dev->pdev->device =3D=3D EHL_Ax_DEVICE_ID) {
> > -               pci_set_power_state(pdev, PCI_D0);
> > -               enable_pme_wake(pdev);
> > -               dev_dbg(dev->devc, "set power state to D0 for
> > ehl\n");
> > -       }
> > -
> >         ish_resume_device =3D device;
> >         dev->resume_flag =3D 1;
> >
> > @@ -400,6 +362,7 @@ static struct pci_driver ish_driver =3D {
> >         .id_table =3D ish_pci_tbl,
> >         .probe =3D ish_probe,
> >         .remove =3D ish_remove,
> > +       .shutdown =3D ish_shutdown,
> >         .driver.pm =3D &ish_pm_ops,
> >  };
> >
>