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

[Lukas Wunner <lukas@wunner.de>]

Hi,

On Wed, May 25, 2016 at 06:04:56PM +0300, Mika Westerberg wrote:
> The PCI PM 1.2 specification requires minimum of 50ms before function on a
> bus is accessed after the bus is transitioned from B2 to B0. Now that we
> actually power down bridges we should make sure the specification is
> followed accordingly.

This patch has the unfortunate side effect of increasing boot time on
Macs with Thunderbolt by 320 ms. Granted, it's not much, but still
noticeable:

[    2.358041] pcieport 0000:06:03.0: enabling device (0000 -> 0002)
[    2.358195] pcieport 0000:06:03.0: rpm_idle
[    2.358222] pcieport 0000:06:03.0: rpm_suspend
[    2.411664] pcieport 0000:06:04.0: enabling device (0000 -> 0003)
[    2.411821] pcieport 0000:06:04.0: rpm_idle
[    2.411848] pcieport 0000:06:04.0: rpm_suspend
[    2.467664] pcieport 0000:06:05.0: enabling device (0000 -> 0003)
[    2.467817] pcieport 0000:06:05.0: rpm_idle
[    2.467843] pcieport 0000:06:05.0: rpm_suspend
[    2.523664] pcieport 0000:06:06.0: enabling device (0000 -> 0002)
[    2.523822] pcieport 0000:06:06.0: rpm_idle
[    2.523848] pcieport 0000:06:06.0: rpm_suspend
[    2.579685] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
[    2.579722] pcieport 0000:06:03.0: rpm_resume
[    2.635750] pciehp 0000:06:03.0:pcie204: Slot #3 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.635853] pciehp 0000:06:03.0:pcie204: service driver pciehp loaded
[    2.635858] pcieport 0000:06:03.0: rpm_idle
[    2.635886] pcieport 0000:06:04.0: rpm_resume
[    2.647645] pcieport 0000:06:03.0: rpm_suspend
[    2.691747] pciehp 0000:06:04.0:pcie204: Slot #4 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.691856] pciehp 0000:06:04.0:pcie204: service driver pciehp loaded
[    2.691859] pcieport 0000:06:04.0: rpm_idle
[    2.691888] pcieport 0000:06:05.0: rpm_resume
[    2.703649] pcieport 0000:06:04.0: rpm_suspend
[    2.747748] pciehp 0000:06:05.0:pcie204: Slot #5 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.747842] pciehp 0000:06:05.0:pcie204: service driver pciehp loaded
[    2.747845] pcieport 0000:06:06.0: rpm_resume
[    2.749213] pcieport 0000:06:05.0: rpm_idle
[    2.759650] pcieport 0000:06:05.0: rpm_suspend
[    2.805049] pciehp 0000:06:06.0:pcie204: Slot #6 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.806464] pciehp 0000:06:06.0:pcie204: service driver pciehp loaded
[    2.806471] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
[    2.807876] intel_idle: MWAIT substates: 0x21120
[    2.807878] intel_idle: v0.4.1 model 0x3A
[    2.808170] intel_idle: lapic_timer_reliable_states 0xffffffff
[    2.808201] GHES: HEST is not enabled!
[    2.809613] pcieport 0000:06:06.0: rpm_idle
[    2.809644] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    2.810096] Linux agpgart interface v0.103
[    2.810158] AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
[    2.810158] AMD IOMMUv2 functionality not available on this system
[    2.816468] pcieport 0000:06:06.0: rpm_suspend

I've added debug messages whenever ->runtime_idle / _suspend / _resume
is called for a device.

As can be seen, the PCI core now waits 50 ms after ->runtime_suspend.
Also, the ports are resumed when the pciehp service driver is loaded,
adding another 50 ms delay. For four hotplug ports, that's a total of
400 ms (versus 80 ms without the patch).

I'm wondering if the delay after ->runtime_suspend is really needed. Is
this mandated by the spec? We could perhaps increase the autosuspend_delay
in pcie_portdrv_probe() slightly to prevent the port from going to sleep
between pci_enable_device() and loading the pciehp service driver.

Thanks,

Lukas

> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi Bjorn,
> 
> Since this is only needed when bridges are powered down, I think it makes
> sense to put this to your pci/pm branch.
> 
>  drivers/pci/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e785dc260e72..e645a3d4f2e0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2361,7 +2361,12 @@ void pci_pm_init(struct pci_dev *dev)
>  	}
>  
>  	dev->pm_cap = pm;
> -	dev->d3_delay = PCI_PM_D3_WAIT;
> +	/*
> +	 * PCI PM 1.2 specification requires minimum of 50ms before any
> +	 * function on the bus is accessed after the bus is transitioned
> +	 * from B2 to B0.
> +	 */
> +	dev->d3_delay = pci_is_bridge(dev) ? PCI_PM_BUS_WAIT : PCI_PM_D3_WAIT;
>  	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;
> -- 
> 2.8.1
>