| Message ID | 1614615911-18794-5-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] mhi: pci_generic: Parametrable element count for events | expand |
On Mon, Mar 01, 2021 at 05:25:10PM +0100, Loic Poulain wrote: > The PCI core can take care of proper PCI suspend/resume operations, but > this is discarded when the driver saves PCI state by its own. This > currently prevents the PCI core to enable PME (for modem initiated > D3 exit) which is requested for proper runtime pm support. > > This change deletes explicit PCI state-saving and state-set from > suspend callback, letting the PCI doing the appropriate work. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/bus/mhi/pci_generic.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c > index 8423293..2a66f80 100644 > --- a/drivers/bus/mhi/pci_generic.c > +++ b/drivers/bus/mhi/pci_generic.c > @@ -544,9 +544,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > pci_set_drvdata(pdev, mhi_pdev); > > - /* Have stored pci confspace at hand for restore in sudden PCI error */ > + /* Have stored pci confspace at hand for restore in sudden PCI error. > + * cache the state locally and discard the PCI core one. > + */ > pci_save_state(pdev); > mhi_pdev->pci_state = pci_store_saved_state(pdev); > + pci_load_saved_state(pdev, NULL); Why do you need to load the state here? Thanks, Mani > > pci_enable_pcie_error_reporting(pdev); > > @@ -717,10 +720,8 @@ static int __maybe_unused mhi_pci_suspend(struct device *dev) > /* Transition to M3 state */ > mhi_pm_suspend(mhi_cntrl); > > - pci_save_state(pdev); > pci_disable_device(pdev); > pci_wake_from_d3(pdev, true); > - pci_set_power_state(pdev, PCI_D3hot); > > return 0; > } > @@ -732,14 +733,13 @@ static int __maybe_unused mhi_pci_resume(struct device *dev) > struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; > int err; > > - pci_set_power_state(pdev, PCI_D0); > - pci_restore_state(pdev); > - pci_set_master(pdev); > - > err = pci_enable_device(pdev); > if (err) > goto err_recovery; > > + pci_set_master(pdev); > + pci_wake_from_d3(pdev, false); > + > /* Exit M3, transition to M0 state */ > err = mhi_pm_resume(mhi_cntrl); > if (err) { > -- > 2.7.4 >
On Thu, 4 Mar 2021 at 07:56, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Mon, Mar 01, 2021 at 05:25:10PM +0100, Loic Poulain wrote: > > The PCI core can take care of proper PCI suspend/resume operations, but > > this is discarded when the driver saves PCI state by its own. This > > currently prevents the PCI core to enable PME (for modem initiated > > D3 exit) which is requested for proper runtime pm support. > > > > This change deletes explicit PCI state-saving and state-set from > > suspend callback, letting the PCI doing the appropriate work. > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > --- > > drivers/bus/mhi/pci_generic.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c > > index 8423293..2a66f80 100644 > > --- a/drivers/bus/mhi/pci_generic.c > > +++ b/drivers/bus/mhi/pci_generic.c > > @@ -544,9 +544,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > pci_set_drvdata(pdev, mhi_pdev); > > > > - /* Have stored pci confspace at hand for restore in sudden PCI error */ > > + /* Have stored pci confspace at hand for restore in sudden PCI error. > > + * cache the state locally and discard the PCI core one. > > + */ > > pci_save_state(pdev); > > mhi_pdev->pci_state = pci_store_saved_state(pdev); > > + pci_load_saved_state(pdev, NULL); > > Why do you need to load the state here? This one corresponds to 'discarding of PCI core saved state', indeed in this driver context we save the PCI config in order to restore it after reset or crash (and only for this). But pci_save_state() is also used 'manually' in some drivers for suspend/resume operations, and when PCI core detects a saved state done by a driver, it considers the PCI driver to have done all appropriated suspend actions and does not interfere in the suspend path. In this driver, we do want PCI core to take care of suspending in a generic way (save-state, enable PME, wakeup, etc...) but for this, we need to be sure the state is not considered as manually saved, so this trick (loading a NULL state), is just used to discard the saved_state from PCI core, to prevent issue on first suspend. Regards, Loic
diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c index 8423293..2a66f80 100644 --- a/drivers/bus/mhi/pci_generic.c +++ b/drivers/bus/mhi/pci_generic.c @@ -544,9 +544,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) pci_set_drvdata(pdev, mhi_pdev); - /* Have stored pci confspace at hand for restore in sudden PCI error */ + /* Have stored pci confspace at hand for restore in sudden PCI error. + * cache the state locally and discard the PCI core one. + */ pci_save_state(pdev); mhi_pdev->pci_state = pci_store_saved_state(pdev); + pci_load_saved_state(pdev, NULL); pci_enable_pcie_error_reporting(pdev); @@ -717,10 +720,8 @@ static int __maybe_unused mhi_pci_suspend(struct device *dev) /* Transition to M3 state */ mhi_pm_suspend(mhi_cntrl); - pci_save_state(pdev); pci_disable_device(pdev); pci_wake_from_d3(pdev, true); - pci_set_power_state(pdev, PCI_D3hot); return 0; } @@ -732,14 +733,13 @@ static int __maybe_unused mhi_pci_resume(struct device *dev) struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; int err; - pci_set_power_state(pdev, PCI_D0); - pci_restore_state(pdev); - pci_set_master(pdev); - err = pci_enable_device(pdev); if (err) goto err_recovery; + pci_set_master(pdev); + pci_wake_from_d3(pdev, false); + /* Exit M3, transition to M0 state */ err = mhi_pm_resume(mhi_cntrl); if (err) {
The PCI core can take care of proper PCI suspend/resume operations, but this is discarded when the driver saves PCI state by its own. This currently prevents the PCI core to enable PME (for modem initiated D3 exit) which is requested for proper runtime pm support. This change deletes explicit PCI state-saving and state-set from suspend callback, letting the PCI doing the appropriate work. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/pci_generic.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) -- 2.7.4