On Tue, Oct 29, 2019 at 01:15:20PM +0200, Mika Westerberg wrote:
> On Mon, Oct 28, 2019 at 03:16:53PM -0500, Bjorn Helgaas wrote:
> > > The related hardware event is resume in this case. Can you point
> > > me to the actual point where you want me to put this?
> >
> > "Resume" is a Linux software concept, so of course the PCIe spec
> > doesn't say anything about it. The spec talks about delays
> > related to resets and device power and link state transitions, so
> > somehow we have to connect the Linux delay with those hardware
> > events.
> >
> > Since we're talking about a transition from D3cold, this has to be
> > done via something external to the device such as power
> > regulators. For ACPI systems that's probably hidden inside _PS0
> > or something similar. That's opaque, but at least it's a hook
> > that says "here's where we put the device into D0". I suggested
> > acpi_pci_set_power_state() as a possibility since I think that's
> > the lowest-level point where we have the pci_dev so we know the
> > current state and the new state.
>
> I looked at how we could use acpi_pci_set_power_state() but I don't
> think it is possible because it is likely that only the root port
> has the power resource that is used to bring the link to L2 or L3.
> However, we would need to repeat the delay for each downstream/root
> port if there are multiple PCIe switches in the topology.
OK, I think I understand why that's a problem (correct me if I'm
wrong):
We call pci_pm_resume_noirq() for every device, but it only calls
acpi_pci_set_power_state() for devices that have _PS0 or _PR0
methods. So if the delay is in acpi_pci_set_power_state() and we
have A -> B -> C where only A has _PS0, we would delay for the link
to B to come up, but not for the link to C.
I do see that we do need both delays. In acpi_pci_set_power_state()
when we transition A from D3cold->D0, I assume that single _PS0
evaluation on A causes B to transition from D3cold->D3hot, which in
turn causes C to transition from D3cold->D3hot. Is that your
understanding, too?
We do know that topology in acpi_pci_set_power_state(), since we have
the pci_dev for A, so it seems conceivable that we could descend the
hierarchy and delay for each level.
If the delay is in pci_pm_resume_noirq() (as in your patch), what
happens with a switch with several Downstream Ports? I assume that
all the Downstream Ports start their transition out of D3cold
basically simultaneously, so we probably don't need N delays, do we?
It seems a little messy to optimize this in pci_pm_resume_noirq().
The outline of the pci_pm_resume_noirq() part of this patch is:
pci_pm_resume_noirq
if (!dev->skip_bus_pm ...) # <-- condition 1
pci_pm_default_resume_early
pci_power_up
if (platform_pci_power_manageable()) # _PS0 or _PR0 exist?
platform_pci_set_power_state
pci_platform_pm->set_state
acpi_pci_set_power_state(PCI_D0) # acpi_pci_platform_pm.set_state
acpi_device_set_power(ACPI_STATE_D0) # <-- eval _PS0
+ if (d3cold) # <-- condition 2
+ pci_bridge_wait_for_secondary_bus
Another thing that niggles at me here is that the condition for
calling pci_bridge_wait_for_secondary_bus() is completely different
than the condition for changing the power state. If we didn't change
the power state, there's no reason to wait, is there?
The outline of the pci_pm_runtime_resume() part of this patch is:
pci_pm_runtime_resume
pci_restore_standard_config
if (dev->current_state != PCI_D0)
pci_set_power_state(PCI_D0)
__pci_start_power_transition
pci_platform_power_transition
if (platform_pci_power_manageable()) # _PS0 or _PR0 exist?
platform_pci_set_power_state
pci_platform_pm->set_state
acpi_pci_set_power_state(PCI_D0) # acpi_pci_platform_pm.set_state
acpi_device_set_power(ACPI_STATE_D0) # <-- eval _PS0
pci_raw_set_power_state
__pci_complete_power_transition
+ if (d3cold)
+ pci_bridge_wait_for_secondary_bus
In this part, the power state change is inside
pci_restore_standard_config(), which calls pci_set_power_state().
There are many other callers of pci_set_power_state(); can we be sure
that none of them need a delay?
> > But it seems that at least some ACPI firmware doesn't do those
> > delays, so I guess our only alternatives are to always do it in
> > the OS or have some sort of blacklist. And it doesn't really seem
> > practical to maintain a blacklist.
>
> I really think this is crystal clear:
I am agreeing with you that the OS needs to do the delays.
> The OS is always responsible for the delays described in the PCIe
> spec.
If the ACPI spec contained this statement, it would be useful, but I
haven't seen it. It's certainly true that some combination of
firmware and the OS is responsible for the delays :)
> However, if the platform implements some of them say in _ON or _PS0
> methods then it can notify the OS about this by using the _DSM so
> the OS does not need to duplicate all of them.
That makes good sense, but there are other reasons for using that
_DSM, e.g., firmware may know that MID or similar devices are not
really PCI devices and don't need delays anywhere. So the existence
of the _DSM by itself doesn't convince me that the OS is responsible
for the delays.
> > > > In pci_pm_reset(), we're doing the D0->D3hot->D0 transitions
> > > > specifically to do a reset, so No_Soft_Reset is false.
> > > > Doesn't 6.6.1 say we need at least 100ms here?
> > >
> > > No since it does not go into D3cold. It just "reset" the thing
> > > if it happens to do internal reset after D3hot -> D0.
> >
> > Sec 5.8, Figure 5-18 says D3hot->D0uninitialized is a "Soft
> > Reset", which unfortunately is not defined.
> >
> > My guess is that in sec 5.9, Table 5-13, the 10ms delay is for the
> > D3hot->D0active (i.e., No_Soft_Reset=1) transition, and the
> > D3hot->D0uninitialized (i.e., No_Soft_Reset=0) that does a "soft
> > reset" (whatever that is) probably requires more and we should
> > handle it like a conventional reset to be safe.
>
> I think it simply means the device functional context is lost (there
> is more in section 5.3.1.4). Linux handles this properly already
> (well at least according the minimum timings required by the spec)
> and restores the context accordingly after it has waited for the
> 10ms.
>
> It is the D3cold (where links go to L2 or L3) where we really need
> the delays so that the link gets properly trained before we start
> poking the downstream device.
I'm already speculating above, so I don't think I can contribute
anything useful here.
