| Message ID | 1425287344-30594-2-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 2015-03-02 at 10:09 +0100, Michal Kazior wrote: > The check isn't really necessary and couldn't even > work because becase pci_restore_state() restores > only first 64 bytes of PCI configuration space. > > This is necessary for future WoWLAN support. > + pci_save_state(pdev); > + pci_disable_device(pdev); I believe the whole thing isn't necessary at all since the kernel's PCIe layer will take care of it. johannes
On 2 March 2015 at 10:16, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2015-03-02 at 10:09 +0100, Michal Kazior wrote: >> The check isn't really necessary and couldn't even >> work because becase pci_restore_state() restores >> only first 64 bytes of PCI configuration space. >> >> This is necessary for future WoWLAN support. > >> + pci_save_state(pdev); >> + pci_disable_device(pdev); > > I believe the whole thing isn't necessary at all since the kernel's PCIe > layer will take care of it. You mean only the pci_save_state() and pci_disable_device() or, the resume counterpart as well? Hmm. Earlier the resume counterpart didn't do any restore stuff and the device ended up pretty confused.. Micha?
On Mon, 2015-03-02 at 10:23 +0100, Michal Kazior wrote: > > I believe the whole thing isn't necessary at all since the kernel's PCIe > > layer will take care of it. > > You mean only the pci_save_state() and pci_disable_device() or, the > resume counterpart as well? Hmm. Resume as well - but I believe it's all-or-nothing. > Earlier the resume counterpart didn't do any restore stuff and the > device ended up pretty confused.. which may explain this. johannes
On 2 March 2015 at 10:25, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2015-03-02 at 10:23 +0100, Michal Kazior wrote: > >> > I believe the whole thing isn't necessary at all since the kernel's PCIe >> > layer will take care of it. >> >> You mean only the pci_save_state() and pci_disable_device() or, the >> resume counterpart as well? Hmm. > > Resume as well - but I believe it's all-or-nothing. > >> Earlier the resume counterpart didn't do any restore stuff and the >> device ended up pretty confused.. > > which may explain this. Hmm.. I removed the save/restore/enable/disable completely and re-tested. It seems to work fine as well. I'll post a v2 later then. Thanks! Micha?
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index cbf82ff..d06b264 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -2053,24 +2053,15 @@ static void ath10k_pci_hif_power_down(struct ath10k *ar) #ifdef CONFIG_PM -#define ATH10K_PCI_PM_CONTROL 0x44 - static int ath10k_pci_hif_suspend(struct ath10k *ar) { struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); struct pci_dev *pdev = ar_pci->pdev; - u32 val; ath10k_pci_sleep(ar); - pci_read_config_dword(pdev, ATH10K_PCI_PM_CONTROL, &val); - - if ((val & 0x000000ff) != 0x3) { - pci_save_state(pdev); - pci_disable_device(pdev); - pci_write_config_dword(pdev, ATH10K_PCI_PM_CONTROL, - (val & 0xffffff00) | 0x03); - } + pci_save_state(pdev); + pci_disable_device(pdev); return 0; } @@ -2088,22 +2079,16 @@ static int ath10k_pci_hif_resume(struct ath10k *ar) return ret; } - pci_read_config_dword(pdev, ATH10K_PCI_PM_CONTROL, &val); + pci_restore_state(pdev); - if ((val & 0x000000ff) != 0) { - pci_restore_state(pdev); - pci_write_config_dword(pdev, ATH10K_PCI_PM_CONTROL, - val & 0xffffff00); - /* - * Suspend/Resume resets the PCI configuration space, - * so we have to re-disable the RETRY_TIMEOUT register (0x41) - * to keep PCI Tx retries from interfering with C3 CPU state - */ - pci_read_config_dword(pdev, 0x40, &val); - - if ((val & 0x0000ff00) != 0) - pci_write_config_dword(pdev, 0x40, val & 0xffff00ff); - } + /* Suspend/Resume resets the PCI configuration space, so we have to + * re-disable the RETRY_TIMEOUT register (0x41) to keep PCI Tx retries + * from interfering with C3 CPU state. pci_restore_state won't help + * here since it only restores the first 64 bytes pci config header. + */ + pci_read_config_dword(pdev, 0x40, &val); + if ((val & 0x0000ff00) != 0) + pci_write_config_dword(pdev, 0x40, val & 0xffff00ff); return ret; }
The check isn't really necessary and couldn't even work because becase pci_restore_state() restores only first 64 bytes of PCI configuration space. This is necessary for future WoWLAN support. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- drivers/net/wireless/ath/ath10k/pci.c | 37 +++++++++++------------------------ 1 file changed, 11 insertions(+), 26 deletions(-)