diff mbox

PCI: Wait for 50ms after bridge is powered up

Message ID 20160531104051.GA13958@wunner.de
State Not Applicable
Headers show

Commit Message

Lukas Wunner May 31, 2016, 10:40 a.m. UTC 
On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> To summarize the next steps. I will send new version of the
> PCI PM patches with following changes.
> 
>   - Drop this 50ms patch, we should have the PCIe 100ms delay already
>     covered.
> 
>   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
>     is the prefered default).

I did some tests, turns out the autosuspend delay need not be increased
to prevent the Thunderbolt hotplug ports from suspending between
"enabling device" and loading the pciehp driver, however the following
is needed:


With this small change things look much better (with the 10 ms delay
we have now):

[    2.353643] pcieport 0000:06:03.0: enabling device (0000 -> 0002)
[    2.353789] pcieport 0000:06:03.0: rpm_idle
[    2.353825] pcieport 0000:06:04.0: enabling device (0000 -> 0003)
[    2.353855] pcieport 0000:06:03.0: rpm_idle
[    2.353994] pcieport 0000:06:04.0: rpm_idle
[    2.354017] pcieport 0000:06:04.0: rpm_idle
[    2.354042] pcieport 0000:06:05.0: enabling device (0000 -> 0003)
[    2.354186] pcieport 0000:06:05.0: rpm_idle
[    2.354213] pcieport 0000:06:06.0: enabling device (0000 -> 0002)
[    2.354362] pcieport 0000:06:06.0: rpm_idle
[    2.354407] pci_hotplug: PCI Hot Plug PCI Core version: 0.5
[    2.354441] pciehp 0000:06:03.0:pcie204: Slot #3 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354524] pciehp 0000:06:03.0:pcie204: service driver pciehp loaded
[    2.354533] pciehp 0000:06:04.0:pcie204: Slot #4 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354609] pciehp 0000:06:04.0:pcie204: service driver pciehp loaded
[    2.354617] pciehp 0000:06:05.0:pcie204: Slot #5 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354690] pciehp 0000:06:05.0:pcie204: service driver pciehp loaded
[    2.354698] pciehp 0000:06:06.0:pcie204: Slot #6 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
[    2.354772] pciehp 0000:06:06.0:pcie204: service driver pciehp loaded
[    2.354777] pciehp: PCI Express Hot Plug Controller Driver version: 0.4
[    2.354827] intel_idle: MWAIT substates: 0x21120
[    2.354829] intel_idle: v0.4.1 model 0x3A
[    2.355122] intel_idle: lapic_timer_reliable_states 0xffffffff
[    2.355152] GHES: HEST is not enabled!
[    2.355216] pcieport 0000:06:05.0: rpm_idle
[    2.355235] pcieport 0000:06:06.0: rpm_idle
[    2.355238] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    2.355277] pcieport 0000:06:03.0: rpm_idle
[    2.355301] pcieport 0000:06:04.0: rpm_idle
[    2.355659] Linux agpgart interface v0.103
[    2.355739] AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
[    2.355764] AMD IOMMUv2 functionality not available on this system
[    2.356396] i8042: PNP: No PS/2 controller found. Probing ports directly.
[    2.364794] pcieport 0000:06:06.0: rpm_suspend
[    2.366112] pcieport 0000:06:05.0: rpm_suspend
[    2.367402] pcieport 0000:06:04.0: rpm_suspend
[    2.368671] pcieport 0000:06:03.0: rpm_suspend

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mika Westerberg May 31, 2016, 10:47 a.m. UTC | #1 
On Tue, May 31, 2016 at 12:40:51PM +0200, Lukas Wunner wrote:
> On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> > To summarize the next steps. I will send new version of the
> > PCI PM patches with following changes.
> > 
> >   - Drop this 50ms patch, we should have the PCIe 100ms delay already
> >     covered.
> > 
> >   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
> >     is the prefered default).
> 
> I did some tests, turns out the autosuspend delay need not be increased
> to prevent the Thunderbolt hotplug ports from suspending between
> "enabling device" and loading the pciehp driver, however the following
> is needed:
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 7860ab3..1d1fb1c 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
>  		pm_runtime_use_autosuspend(&dev->dev);
>  		pm_runtime_put_autosuspend(&dev->dev);
> +		pm_runtime_mark_last_busy(&dev->dev);
>  		pm_runtime_allow(&dev->dev);
>  	}

I still prefer increasing the autosuspend delay. The above looks hackish
and does not work if it takes more than 10ms to get to the tbt driver
probe.

Did you try if it also works with 500ms delay?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner May 31, 2016, 11:07 a.m. UTC | #2 
On Tue, May 31, 2016 at 01:47:14PM +0300, Mika Westerberg wrote:
> On Tue, May 31, 2016 at 12:40:51PM +0200, Lukas Wunner wrote:
> > On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> > > To summarize the next steps. I will send new version of the
> > > PCI PM patches with following changes.
> > > 
> > >   - Drop this 50ms patch, we should have the PCIe 100ms delay already
> > >     covered.
> > > 
> > >   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
> > >     is the prefered default).
> > 
> > I did some tests, turns out the autosuspend delay need not be increased
> > to prevent the Thunderbolt hotplug ports from suspending between
> > "enabling device" and loading the pciehp driver, however the following
> > is needed:
> > 
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index 7860ab3..1d1fb1c 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >  		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
> >  		pm_runtime_use_autosuspend(&dev->dev);
> >  		pm_runtime_put_autosuspend(&dev->dev);
> > +		pm_runtime_mark_last_busy(&dev->dev);
> >  		pm_runtime_allow(&dev->dev);
> >  	}
> 
> I still prefer increasing the autosuspend delay. The above looks hackish
> and does not work if it takes more than 10ms to get to the tbt driver
> probe.
> 
> Did you try if it also works with 500ms delay?

I tried 150 ms and it didn't work. The pm_runtime_mark_last_busy()
is needed no matter how much you increase the autosuspend delay.
This isn't hackish. :-) The issue is that pm_runtime_mark_last_busy()
has never been called so far, so dev->power.last_busy == 0.
The PM core thinks that the device can suspend right away because it's
last been busy more than 2 seconds ago.

One could argue though if pm_runtime_mark_last_busy() should come
before pm_runtime_put_autosuspend(). Usually it should, but in this
case it doesn't matter because pm_runtime_allow() hasn't been called
yet and the PCI core initializes devices to pm_runtime_forbid().

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg June 1, 2016, 9:11 a.m. UTC | #3 
On Tue, May 31, 2016 at 01:07:57PM +0200, Lukas Wunner wrote:
> On Tue, May 31, 2016 at 01:47:14PM +0300, Mika Westerberg wrote:
> > On Tue, May 31, 2016 at 12:40:51PM +0200, Lukas Wunner wrote:
> > > On Tue, May 31, 2016 at 11:58:05AM +0300, Mika Westerberg wrote:
> > > > To summarize the next steps. I will send new version of the
> > > > PCI PM patches with following changes.
> > > > 
> > > >   - Drop this 50ms patch, we should have the PCIe 100ms delay already
> > > >     covered.
> > > > 
> > > >   - Increase runtime PM autosuspend time from 10ms to 500ms (or whatever
> > > >     is the prefered default).
> > > 
> > > I did some tests, turns out the autosuspend delay need not be increased
> > > to prevent the Thunderbolt hotplug ports from suspending between
> > > "enabling device" and loading the pciehp driver, however the following
> > > is needed:
> > > 
> > > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > > index 7860ab3..1d1fb1c 100644
> > > --- a/drivers/pci/pcie/portdrv_pci.c
> > > +++ b/drivers/pci/pcie/portdrv_pci.c
> > > @@ -238,6 +238,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > >  		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
> > >  		pm_runtime_use_autosuspend(&dev->dev);
> > >  		pm_runtime_put_autosuspend(&dev->dev);
> > > +		pm_runtime_mark_last_busy(&dev->dev);
> > >  		pm_runtime_allow(&dev->dev);
> > >  	}
> > 
> > I still prefer increasing the autosuspend delay. The above looks hackish
> > and does not work if it takes more than 10ms to get to the tbt driver
> > probe.
> > 
> > Did you try if it also works with 500ms delay?
> 
> I tried 150 ms and it didn't work. The pm_runtime_mark_last_busy()
> is needed no matter how much you increase the autosuspend delay.
> This isn't hackish. :-) The issue is that pm_runtime_mark_last_busy()
> has never been called so far, so dev->power.last_busy == 0.

Yes, it looks like it is really needed. I somehow remembered that
pm_runtime_set_autosuspend_delay() sets that automatically. So I take
back my hackish comment ;-)

> The PM core thinks that the device can suspend right away because it's
> last been busy more than 2 seconds ago.

Right.

> One could argue though if pm_runtime_mark_last_busy() should come
> before pm_runtime_put_autosuspend(). Usually it should, but in this
> case it doesn't matter because pm_runtime_allow() hasn't been called
> yet and the PCI core initializes devices to pm_runtime_forbid().

I'm going to change the code to look like following (pm_runtime_mark_last_busy()
gets called before pm_runtime_put_autosuspend() even if not strictly needed):

	pm_runtime_set_autosuspend_delay(&dev->dev, 100);
	pm_runtime_use_autosuspend(&dev->dev);
	pm_runtime_mark_last_busy(&dev->dev);
	pm_runtime_put_autosuspend(&dev->dev);
	pm_runtime_allow(&dev->dev);

Note I'm still increasing default autosuspend delay from 10ms to 100ms.

Does the above work for you?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner June 1, 2016, 11:42 a.m. UTC | #4 
On Wed, Jun 01, 2016 at 12:11:45PM +0300, Mika Westerberg wrote:
> I'm going to change the code to look like following (pm_runtime_mark_last_busy()
> gets called before pm_runtime_put_autosuspend() even if not strictly needed):
> 
> 	pm_runtime_set_autosuspend_delay(&dev->dev, 100);
> 	pm_runtime_use_autosuspend(&dev->dev);
> 	pm_runtime_mark_last_busy(&dev->dev);
> 	pm_runtime_put_autosuspend(&dev->dev);
> 	pm_runtime_allow(&dev->dev);
> 
> Note I'm still increasing default autosuspend delay from 10ms to 100ms.
> 
> Does the above work for you?

Yes, tested it and couldn't spot any issues.

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 7860ab3..1d1fb1c 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -238,6 +238,7 @@  static int pcie_portdrv_probe(struct pci_dev *dev,
 		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
 		pm_runtime_use_autosuspend(&dev->dev);
 		pm_runtime_put_autosuspend(&dev->dev);
+		pm_runtime_mark_last_busy(&dev->dev);
 		pm_runtime_allow(&dev->dev);
 	}