On Tue, Nov 05, 2019 at 02:58:18PM +0200, Mika Westerberg wrote:
> On Tue, Nov 05, 2019 at 11:54:33AM +0200, Mika Westerberg wrote:
> > > For understandability, I think the wait needs to go in some function
> > > that contains "PCI_D0", e.g., platform_pci_set_power_state() or
> > > pci_power_up(), so it's connected with the transition from D3cold to
> > > D0.
> > >
> > > Since pci_pm_default_resume_early() is the only caller of
> > > pci_power_up(), maybe we should just inline pci_power_up(), e.g.,
> > > something like this:
> > >
> > > static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
> > > {
> > > pci_power_state_t prev_state = pci_dev->current_state;
> > >
> > > if (platform_pci_power_manageable(pci_dev))
> > > platform_pci_set_power_state(pci_dev, PCI_D0);
> > >
> > > pci_raw_set_power_state(pci_dev, PCI_D0);
> > > pci_update_current_state(pci_dev, PCI_D0);
> > >
> > > pci_restore_state(pci_dev);
> > > pci_pme_restore(pci_dev);
> > >
> > > if (prev_state == PCI_D3cold)
> > > pci_bridge_wait_for_secondary_bus(dev);
> > > }
> >
> > OK, I'll see if this works.
>
> Well, I think we want to execute pci_fixup_resume_early before we delay
> for the downstream component (same for runtime resume path). Currently
> nobody is using that for root/downstream ports but in theory at least
> some port may need it before it works properly. Also probably good idea
> to disable wake as soon as possible to avoid possible extra PME messages
> coming from the port.
OK, I wish we could connect it more closely with the actual power-on,
but I guess that makes sense.
> I feel that the following is the right place to perform the delay but if
> you think we can ignore the above, I will just do what you say and do it
> in pci_pm_default_resume_early() (and pci_restore_standard_config() for
> runtime path).
>
> [The below diff does not have check for pci_dev->skip_bus_pm because I
> was planning to move it inside pci_bridge_wait_for_secondary_bus() itself.]
What do you gain by moving it? IIUC we want them to be the same
condition, and if one is in pci_pm_resume_noirq() and another is
inside pci_bridge_wait_for_secondary_bus(), it's hard to see that
they're connected. I'd rather have pci_pm_resume_noirq() check it
once, save the result, and test that result before calling
pci_pm_default_resume_early() and pci_bridge_wait_for_secondary_bus().
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 08d3bdbc8c04..3c0e52aaef79 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -890,6 +890,7 @@ static int pci_pm_resume_noirq(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + pci_power_t prev_state = pci_dev->current_state;
>
> if (dev_pm_may_skip_resume(dev))
> return 0;
> @@ -914,6 +915,9 @@ static int pci_pm_resume_noirq(struct device *dev)
> pci_fixup_device(pci_fixup_resume_early, pci_dev);
> pcie_pme_root_status_cleanup(pci_dev);
>
> + if (prev_state == PCI_D3cold)
> + pci_bridge_wait_for_secondary_bus(pci_dev);
> +
> if (pci_has_legacy_pm_support(pci_dev))
> return 0;
>
> @@ -1299,6 +1303,7 @@ static int pci_pm_runtime_resume(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + pci_power_t prev_state = pci_dev->current_state;
> int error = 0;
>
> /*
> @@ -1314,6 +1319,9 @@ static int pci_pm_runtime_resume(struct device *dev)
> pci_fixup_device(pci_fixup_resume_early, pci_dev);
> pci_pm_default_resume(pci_dev);
>
> + if (prev_state == PCI_D3cold)
> + pci_bridge_wait_for_secondary_bus(pci_dev);
> +
> if (pm && pm->runtime_resume)
> error = pm->runtime_resume(dev);
>
