On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote: > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada > <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote: > > > > 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. > > This patch does exactly the same thing by letting PCI and ACPI handle > the PME and GPE. > Though the change seems to be bigger, it actually reduces the duped > code, while keep the S5 wakeup ability intact. It may be doing the same. But with long chain of calls without verification, I am not comfortable. This can be another patch by itself to use the framework. But you are targeting a fix for overflow issue, which is separate from the use of PCI/ACPI framework. Thanks, Srinivas > > Kai-Heng > > > > > 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, > > > > > > }; > > > > > > > > > > > > >
