Hi Kai-Heng, On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote: > Hi Even, > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@xxxxxxxxx> wrote: > > > > Hi, Kai-Heng, > > > > I just got feedback, for testing EHL S5 wakeup feature, you need > > several steps to setup and access > > "https://portal.devicewise.com/things/browse" to trigger wake. > > But currently, our test account of this website are all out of > > data. > > So maybe you need double check with the team who required you > > preparing the patch for the verification. > > The patch is to solve the GPE refcount overflow, while maintaining S5 > wakeup. I don't have any mean to test S5 wake. > The issue is not calling acpi_disable_gpe(). To reduce the scope of change can we just add that instead of a adding new callbacks. This way scope is reduced. Something like the attached Thanks, Srinivas > So if you also don't have ways to verify S5 wake functionality, maybe > we can simply revert 2e23a70edabe ("HID: intel-ish-hid: ipc: finish > power flow for EHL OOB") as alternative? > > Kai-Heng > > > Thanks! > > > > Best Regards, > > Even Xu > > > > -----Original Message----- > > From: Xu, Even > > Sent: Friday, September 15, 2023 3:27 PM > > To: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > > Cc: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>; > > jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx; > > linux-pm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Lee, Jian Hui > > <jianhui.lee@xxxxxxxxxxxxx>; 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, Kai-Heng, > > > > I am also not familiar with this S5 wakeup test case. > > I already sent out mails to ask for help on it. > > Will come back to you once I get feedback. > > Thanks! > > > > Best Regards, > > Even Xu > > > > -----Original Message----- > > From: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > > Sent: Friday, September 15, 2023 2:01 PM > > To: Xu, Even <even.xu@xxxxxxxxx> > > Cc: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>; > > jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx; > > linux-pm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Lee, Jian Hui > > <jianhui.lee@xxxxxxxxxxxxx>; 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 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, > > > > }; > > > > > > >
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c index 55cb25038e63..0d854e4f1f7c 100644 --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c @@ -119,7 +119,7 @@ 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) +static int enable_gpe(struct device *dev, bool status) { #ifdef CONFIG_ACPI acpi_status acpi_sts; @@ -133,7 +133,11 @@ static int enable_gpe(struct device *dev) } wakeup = &adev->wakeup; - acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number); + if (status) + acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number); + else + acpi_sts = acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); + if (ACPI_FAILURE(acpi_sts)) { dev_err(dev, "enable ose_gpe failed\n"); return -EIO; @@ -145,13 +149,13 @@ static int enable_gpe(struct device *dev) #endif } -static void enable_pme_wake(struct pci_dev *pdev) +static void enable_pme_wake(struct pci_dev *pdev, status) { 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"); + pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev), status) { + pci_pme_active(pdev, status); + dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled state %d\n", status); } } @@ -225,7 +229,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); + enable_pme_wake(pdev, true); ret = ish_init(ishtp); if (ret) @@ -353,6 +357,9 @@ static int __maybe_unused ish_suspend(struct device *device) ish_disable_dma(dev); } + if (dev->pdev->device == EHL_Ax_DEVICE_ID) + enable_pme_wake(pdev, false); + return 0; } @@ -373,7 +380,7 @@ static int __maybe_unused ish_resume(struct device *device) /* 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); + enable_pme_wake(pdev, true); dev_dbg(dev->devc, "set power state to D0 for ehl\n"); }