Hi Kalle,
I have pushed two patches usingn 'git send-mail' and below patch is the first one;
I could not see the patches in patchwork page(https://patchwork.kernel.org/project/linux-wireless/list/); Did you get this patch?
Regards,
Ganapathi
> -----Original Message-----
> From: Ganapathi Bhat <[email protected]>
> Sent: Thursday, May 30, 2019 3:23 PM
> To: [email protected]
> Cc: Cathy Luo <[email protected]>; Zhiyuan Yang <[email protected]>;
> James Cao <[email protected]>; Rakesh Parmar <[email protected]>;
> Sharvari Harisangam <[email protected]>; Ganapathi Bhat
> <[email protected]>
> Subject: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#
>
> From: Sharvari Harisangam <[email protected]>
>
> PCIE WAKE# is asserted by firmware, when WoWLAN conditions are
> matched. Current driver does not enable PME bit needed for WAKE#
> assertion, causing host to remain in sleep even after WoWLAN conditions are
> matched. This commit fixes it by enabling wakeup (PME bit) in suspend
> handler.
>
> Signed-off-by: Sharvari Harisangam <[email protected]>
> Signed-off-by: Ganapathi Bhat <[email protected]>
> ---
> drivers/net/wireless/marvell/mwifiex/pcie.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 3fe81b2..0bd81d4 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -147,11 +147,10 @@ static bool mwifiex_pcie_ok_to_access_hw(struct
> mwifiex_adapter *adapter)
> * If already not suspended, this function allocates and sends a host
> * sleep activate request to the firmware and turns off the traffic.
> */
> -static int mwifiex_pcie_suspend(struct device *dev)
> +static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t
> +state)
> {
> struct mwifiex_adapter *adapter;
> struct pcie_service_card *card;
> - struct pci_dev *pdev = to_pci_dev(dev);
>
> card = pci_get_drvdata(pdev);
>
> @@ -160,7 +159,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
>
> adapter = card->adapter;
> if (!adapter) {
> - dev_err(dev, "adapter is not valid\n");
> + dev_err(&pdev->dev, "adapter is not valid\n");
> return 0;
> }
>
> @@ -181,6 +180,10 @@ static int mwifiex_pcie_suspend(struct device *dev)
> set_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
> clear_bit(MWIFIEX_IS_HS_ENABLING, &adapter->work_flags);
>
> + pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
> + pci_save_state(pdev);
> + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +
> return 0;
> }
>
> @@ -192,16 +195,20 @@ static int mwifiex_pcie_suspend(struct device
> *dev)
> * If already not resumed, this function turns on the traffic and
> * sends a host sleep cancel request to the firmware.
> */
> -static int mwifiex_pcie_resume(struct device *dev)
> +static int mwifiex_pcie_resume(struct pci_dev *pdev)
> {
> struct mwifiex_adapter *adapter;
> struct pcie_service_card *card;
> - struct pci_dev *pdev = to_pci_dev(dev);
> +
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> + pci_enable_wake(pdev, PCI_D0, 0);
> +
>
> card = pci_get_drvdata(pdev);
>
> if (!card->adapter) {
> - dev_err(dev, "adapter structure is not valid\n");
> + dev_err(&pdev->dev, "adapter structure is not valid\n");
> return 0;
> }
>
> @@ -416,11 +423,6 @@ static void mwifiex_pcie_reset_done(struct pci_dev
> *pdev)
> .reset_done = mwifiex_pcie_reset_done,
> };
>
> -#ifdef CONFIG_PM_SLEEP
> -/* Power Management Hooks */
> -static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend,
> - mwifiex_pcie_resume);
> -#endif
>
> /* PCI Device Driver */
> static struct pci_driver __refdata mwifiex_pcie = { @@ -431,7 +433,8 @@
> static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend,
> .driver = {
> .coredump = mwifiex_pcie_coredump,
> #ifdef CONFIG_PM_SLEEP
> - .pm = &mwifiex_pcie_pm_ops,
> + .suspend = mwifiex_pcie_suspend,
> + .resume = mwifiex_pcie_resume,
> #endif
> },
> .shutdown = mwifiex_pcie_shutdown,
> --
> 1.9.1
Ganapathi Bhat <[email protected]> writes:
> I have pushed two patches usingn 'git send-mail' and below patch is the first one;
>
> I could not see the patches in patchwork page(https://patchwork.kernel.org/project/linux-wireless/list/); Did you get this patch?
I take patches directly from patchwork and if it's not there I don't see
it. So you need to resend.
--
Kalle Valo
(You really should re-send your patches, as they didn't make it to the
list. But while you're here:)
On Thu, May 30, 2019 at 3:01 AM Ganapathi Bhat <[email protected]> wrote:
> > From: Sharvari Harisangam <[email protected]>
> >
> > PCIE WAKE# is asserted by firmware, when WoWLAN conditions are
> > matched. Current driver does not enable PME bit needed for WAKE#
> > assertion, causing host to remain in sleep even after WoWLAN conditions are
> > matched. This commit fixes it by enabling wakeup (PME bit) in suspend
> > handler.
Are you sure said devices actually have their 'wakeup' attribute set
to 'enabled' (e.g., in sysfs)? I think the PCI core should probably be
taking care of this for you already. See below.
> > Signed-off-by: Sharvari Harisangam <[email protected]>
> > Signed-off-by: Ganapathi Bhat <[email protected]>
> > ---
> > drivers/net/wireless/marvell/mwifiex/pcie.c | 27 +++++++++++++++------------
> > 1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 3fe81b2..0bd81d4 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
...
> > @@ -181,6 +180,10 @@ static int mwifiex_pcie_suspend(struct device *dev)
> > set_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
> > clear_bit(MWIFIEX_IS_HS_ENABLING, &adapter->work_flags);
> >
> > + pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
> > + pci_save_state(pdev);
> > + pci_set_power_state(pdev, pci_choose_state(pdev, state));
From Documentation/power/pci.txt:
"""
3.1.2. suspend()
...
This callback is expected to quiesce the device and prepare it to be put into a
low-power state by the PCI subsystem. It is not required (in fact it even is
not recommended) that a PCI driver's suspend() callback save the standard
configuration registers of the device, prepare it for waking up the system, or
put it into a low-power state. All of these operations can very well be taken
care of by the PCI subsystem, without the driver's participation.
However, in some rare case it is convenient to carry out these operations in
a PCI driver. [...]
"""
I think you need to do a little more work to justify why you are one
of those rare cases.
On a related note: some of the existing "configure wakeup" stuff we do
here should probably be gated behind a call to device_may_wakeup().
That would help make it more obvious that such configuration is
futile, if the device was marked as wake-disabled.
Brian
> > +
> > return 0;
> > }
> >