Hi Even,
On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@xxxxxxxxx> wrote:
>
> Hi, Srinivas,
>
> Sure, I will test it.
> As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.
I only made sure the GPE overflow issue is fixed by the patch, but I
didn't test the S5 wakeup.
That's because I don't know how to test it on the EHL system I have.
I'll test it if you can let me know how to test the S5 wakeup.
Kai-Heng
> I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
>
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Sent: Friday, September 15, 2023 12:11 AM
> To: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>; jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx
> Cc: linux-pm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Lee, Jian Hui <jianhui.lee@xxxxxxxxxxxxx>; Xu, Even <even.xu@xxxxxxxxx>; Zhang, Lixu <lixu.zhang@xxxxxxxxx>; Ba, Najumon <najumon.ba@xxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > System cannot suspend more than 255 times because the driver doesn't
> > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the
> > GPE refcount overflows.
> >
> > 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.
> >
> Please test this change.
>
> Thanks,
> Srinivas
>
> > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for
> > EHL OOB")
> > Cc: Jian Hui Lee <jianhui.lee@xxxxxxxxxxxxx>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > ---
> > drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59 +++++++----------------
> > --
> > 1 file changed, 15 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 55cb25038e63..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> > pci_dev *pdev)
> > return !pm_resume_via_firmware() || pdev->device ==
> > 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 = ACPI_COMPANION(dev);
> > - if (!adev) {
> > - dev_err(dev, "get acpi handle failed\n");
> > - return -ENODEV;
> > - }
> > - wakeup = &adev->wakeup;
> > -
> > - acpi_sts = 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
> > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> > struct pci_device_id *ent)
> >
> > /* Enable PME for EHL */
> > if (pdev->device == EHL_Ax_DEVICE_ID)
> > - enable_pme_wake(pdev);
> > + device_init_wakeup(dev, true);
> >
> > ret = ish_init(ishtp);
> > if (ret)
> > @@ -248,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 == EHL_Ax_DEVICE_ID)
> > + pci_prepare_to_sleep(pdev); }
> > +
> > static struct device __maybe_unused *ish_resume_device;
> >
> > /* 50ms to get resume response */
> > @@ -370,13 +347,6 @@ static int __maybe_unused ish_resume(struct
> > device *device)
> > struct pci_dev *pdev = to_pci_dev(device);
> > struct ishtp_device *dev = pci_get_drvdata(pdev);
> >
> > - /* add this to finish power flow for EHL */
> > - if (dev->pdev->device == 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 = device;
> > dev->resume_flag = 1;
> >
> > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > .id_table = ish_pci_tbl,
> > .probe = ish_probe,
> > .remove = ish_remove,
> > + .shutdown = ish_shutdown,
> > .driver.pm = &ish_pm_ops,
> > };
> >
>
