Re: [PATCH] PCI: Wait for 50ms after bridge is powered up

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Mon, May 30, 2016 at 05:19:53PM +0200, Andreas Noever wrote:
> On Mon, May 30, 2016 at 4:44 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, May 30, 2016 at 12:33:26PM +0300, Mika Westerberg wrote:
> >> I also learned that both of these can be shortened with following
> >> mechanisms:
> >>
> >>  - PCIe readines notifications (6.23 in PCIe spec 3.1a)
> >>  - ACPI _DSM method returning readines durations from  (4.6.9 in PCI
> >>    firmware spec 3.2).
> >
> > BTW, looks like the latter is already implemented in
> > pci_acpi_optimize_delay().
> 
> 
> Hmm, Lukas's trace suggest a few independent problems (or optimisations):
> 1. Those devices are siblings, we should wake all of them in parallel
> and then wait once instead of one by one.
> 2. Why are we waiting after _suspend at all? It seems we should only
> wait before the next access. Sleeping after _suspend looks like a stop
> gap measure to guarantee that no accesses take place?

I guess it is because PCI PM spec says that the delay needs to be in
both directions. See table 5-6 in PCI PM spec v1.2.

The code in question is in pci_raw_set_power_state():

        /* Mandatory power management transition delays */
        /* see PCI PM 1.1 5.6.1 table 18 */
        if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
                pci_dev_d3_sleep(dev);
        else if (state == PCI_D2 || dev->current_state == PCI_D2)
                udelay(PCI_PM_D2_DELAY);

> 3. The idle timeout is too low, there should be no suspend between
> discovery and probing.

I agree. We took the 10ms from the original patch but I don't see why we
could not increase it to 100ms or even 500ms.

> 4. Even if the spec says 50ms, we might still want to have shorter
> sleep times for known good devices. Thunderbolt can produce PCI
> hierarchies which are 7 levels deep with 4 hp bridges on each level.
> Waking all of that would take 50ms * 7 * 4 = 1400ms (not counting
> upstream bridges which have the normal d3 delay).

PCIe spec want to have 100ms delay from when transitioning from D3cold
to D0 and we already do that in __pci_start_power_transition().

In other words we should have all necessary delays for PCIe in place
already. This patch should not be needed.

For decreasing delays we already support ACPI _DSM method. What is
missing is PCIe readines notification support which can be added
separately.

> Does TB support PCIe readines notifications?

It seems to be pretty recent feature so it might not be supported by the
existing TBT hardware.