Re: [RFC PATCH] PCIe: Add PCIe runtime D3cold support

[huang ying]

Hi,

On Sat, Apr 14, 2012 at 3:41 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> Hi,
>
> On Friday, April 13, 2012, Yan, Zheng wrote:
>> Hi all,
>>
>> This patch adds PCIe runtime D3cold support, namely cut power supply for functions
>> beneath a PCIe port when they all have entered D3. A device in D3cold can only
>> generate wake event through the WAKE# pin. Because we can not access to a device's
>> configure space while it's in D3cold, pme_poll is disabled for devices in D3cold.
>>
>> Any comment will be appreciated.
>>
>> Signed-off-by: Zheng Yan <zheng.z.yan@xxxxxxxxx>
>> ---
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 0f150f2..e210e8cb 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>> Â Â Â Â Â Â Â [PCI_D1] = ACPI_STATE_D1,
>> Â Â Â Â Â Â Â [PCI_D2] = ACPI_STATE_D2,
>> Â Â Â Â Â Â Â [PCI_D3hot] = ACPI_STATE_D3,
>> - Â Â Â Â Â Â [PCI_D3cold] = ACPI_STATE_D3
>> + Â Â Â Â Â Â [PCI_D3cold] = ACPI_STATE_D3_COLD
>> Â Â Â };
>> Â Â Â int error = -EINVAL;
>>
>
> Please don't use that ACPI_STATE_D3_COLD thing, it's not defined correctly.
>
> We should define ACPI_STATE_D3_COLD == ACPI_STATE_D3 and add ACPI_STATE_D3_HOT
> instead. ÂI'll prepare a patch for that over the weekend if no one has done
> that already.
>
>> @@ -296,7 +296,8 @@ static void acpi_pci_propagate_run_wake(struct pci_bus *bus, bool enable)
>>
>> Âstatic int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
>> Â{
>> - Â Â if (dev->pme_interrupt)
>> + Â Â /* PME interrupt isn't available in the D3cold case */
>> + Â Â if (dev->pme_interrupt && !dev->runtime_d3cold)
>
> This whole thing is wrong. ÂFirst off, I don't think that the runtime_d3cold
> flag makes any sense. ÂWe already cover that in dev->pme_support.

PCIe devices and PCIe root port may have proper PME interrupt support
(that is, dev->pme_interrupt = true), but the process of remote wakeup
from D3cold is as follow:

1) In D3cold, the power of the main link is turned off, aux power is
provided (PCIe L2 link state)
2) Device detect condition to resume, then assert #WAKE pin
2) ACPI circuit linked with #WAKE pin, and will generate a GPE for that
3) GPE handler will resume device with ACPI (via _PS3 and _PR0), the
power of the main link is turned on, after a while, link goes into L0
state
4) The PME message is sent to root port, pme interrupt generated

So, for deivce, dev->pme_interrupt = true and dev->pme_support
advocate it support PME in D3cold.  But we still need ACPI to setup
run wake for the device.

So we need a way to inform acpi_pci_run_wake(), one way is using a
flag, there is other ways such as adding another parameter to
acpi_pci_run_wake() to tell it which power state to go.

> Second, pme_interrupt means that the _root_ _port_, not the device itself will
> trigger an interrupt whenever the device sends the PME message to it (which
> very well may happen for a device in D3_cold woken up by an external signal).

As above, that may happend after ACPI GPE.

>> Â Â Â Â Â Â Â return 0;
>>
>> Â Â Â if (!acpi_pm_device_run_wake(&dev->dev, enable))
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 8156744..bc16869 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -731,8 +731,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>> Â Â Â int error;
>>
>
> Guys, please. ÂNever, _ever_, touch pci_set_power_state() without discussing
> your ideas with someone who knows how it works and _why_ it works this way.

This patch is sent as RFC, I think this is to call for discussing
instead of merging, isn't it?

> The problem here is that you can't program a PCI device into D3_cold, so it
> doesn't even make sense to have a helper for that.

We need to program ACPI to make the PCI device go into D3_cold.  That
is called in this function.

pci_set_power_sate
  -> __pci_complete_power_transition
    -> pci_platform_power_transition

>> Â Â Â /* bound the state we're entering */
>> - Â Â if (state > PCI_D3hot)
>> - Â Â Â Â Â Â state = PCI_D3hot;
>> + Â Â if (state > PCI_D3cold)
>> + Â Â Â Â Â Â state = PCI_D3cold;
>> Â Â Â else if (state < PCI_D0)
>> Â Â Â Â Â Â Â state = PCI_D0;
>> Â Â Â else if ((state == PCI_D1 || state == PCI_D2) && pci_no_d1d2(dev))
>> @@ -750,7 +750,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>> Â Â Â if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
>> Â Â Â Â Â Â Â return 0;
>>
>> - Â Â error = pci_raw_set_power_state(dev, state);
>> + Â Â error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â PCI_D3hot : state);
>>
>> Â Â Â if (!__pci_complete_power_transition(dev, state))
>> Â Â Â Â Â Â Â error = 0;
>> @@ -1482,6 +1483,17 @@ bool pci_pme_capable(struct pci_dev *dev, pci_power_t state)
>> Â Â Â return !!(dev->pme_support & (1 << state));
>> Â}
>>
>> +static void pci_pme_poll_wakeup(struct pci_dev *dev)
>> +{
>> + Â Â struct pci_dev *bridge = dev->bus->self;
>> +
>> + Â Â /* don't poll the pme bit if parent is in low power state */
>> + Â Â if (bridge && bridge->current_state != PCI_D0)
>> + Â Â Â Â Â Â return;
>> +
>> + Â Â pci_pme_wakeup(dev, NULL);
>> +}
>
> This one actually makes some sense, although it might be better to put the
> test into pci_pme_wakeup() itself.
>
>> +
>> Âstatic void pci_pme_list_scan(struct work_struct *work)
>> Â{
>> Â Â Â struct pci_pme_device *pme_dev, *n;
>> @@ -1490,7 +1502,7 @@ static void pci_pme_list_scan(struct work_struct *work)
>> Â Â Â if (!list_empty(&pci_pme_list)) {
>> Â Â Â Â Â Â Â list_for_each_entry_safe(pme_dev, n, &pci_pme_list, list) {
>> Â Â Â Â Â Â Â Â Â Â Â if (pme_dev->dev->pme_poll) {
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â pci_pme_wakeup(pme_dev->dev, NULL);
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â pci_pme_poll_wakeup(pme_dev->dev);
>> Â Â Â Â Â Â Â Â Â Â Â } else {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â list_del(&pme_dev->list);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â kfree(pme_dev);
>> @@ -1608,6 +1620,10 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>> Â Â Â if (enable) {
>> Â Â Â Â Â Â Â int error;
>>
>> + Â Â Â Â Â Â if (runtime && state >= PCI_D3cold)
>> + Â Â Â Â Â Â Â Â Â Â dev->runtime_d3cold = true;
>> + Â Â Â Â Â Â else
>> + Â Â Â Â Â Â Â Â Â Â dev->runtime_d3cold = false;
>> Â Â Â Â Â Â Â if (pci_pme_capable(dev, state))
>> Â Â Â Â Â Â Â Â Â Â Â pci_pme_active(dev, true);
>> Â Â Â Â Â Â Â else
>> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
>> index e0610bd..d66b7e9 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -11,11 +11,13 @@
>> Â#include <linux/kernel.h>
>> Â#include <linux/errno.h>
>> Â#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> Â#include <linux/init.h>
>> Â#include <linux/pcieport_if.h>
>> Â#include <linux/aer.h>
>> Â#include <linux/dmi.h>
>> Â#include <linux/pci-aspm.h>
>> +#include <linux/delay.h>
>>
>> Â#include "portdrv.h"
>> Â#include "aer/aerdrv.h"
>> @@ -99,6 +101,25 @@ static int pcie_port_resume_noirq(struct device *dev)
>> Â Â Â return 0;
>> Â}
>>
>> +static int pcie_port_runtime_suspend(struct device *dev)
>> +{
>> + Â Â struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> + Â Â pci_save_state(pdev);
>
> Are you sure this is sufficient?

Maybe we need to pay attention to the pcie port drivers?  Run ->runtime_

>> + Â Â return 0;
>> +}
>> +
>> +static int pcie_port_runtime_resume(struct device *dev)
>> +{
>> + Â Â struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> + Â Â pci_restore_state(pdev);
>> + Â Â if (pdev->runtime_d3cold)
>> + Â Â Â Â Â Â msleep(100);
>
> What's _that_ supposed to do?

When resume from d3cold, PCIe main link will be powered on again, it
will take quite some time before the main link go into L0 state.
Otherwise, accessing devices under the port may return wrong result.

>> +
>> + Â Â return 0;
>> +}
>> +
>> Âstatic const struct dev_pm_ops pcie_portdrv_pm_ops = {
>>    .suspend    Â= pcie_port_device_suspend,
>>    .resume     = pcie_port_device_resume,
>> @@ -107,6 +128,8 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>>    .poweroff    = pcie_port_device_suspend,
>>    .restore    Â= pcie_port_device_resume,
>>    .resume_noirq  = pcie_port_resume_noirq,
>> + Â Â .runtime_suspend = pcie_port_runtime_suspend,
>> + Â Â .runtime_resume = pcie_port_runtime_resume,
>> Â};
>>
>> Â#define PCIE_PORTDRV_PM_OPS Â(&pcie_portdrv_pm_ops)
>> @@ -144,12 +167,14 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
>> Â Â Â Â Â Â Â return status;
>>
>> Â Â Â pci_save_state(dev);
>> + Â Â pm_runtime_put_noidle(&dev->dev);
>
> What's the purpose of this?

I think this is needed by PCI runtime PM support.  Before ->probe,
pm_runtime_get_xxx() is executed on device.

>> Â Â Â return 0;
>> Â}
>>
>> Âstatic void pcie_portdrv_remove(struct pci_dev *dev)
>> Â{
>> + Â Â pm_runtime_get_noresume(&dev->dev);
>> Â Â Â pcie_port_device_remove(dev);
>> Â Â Â pci_disable_device(dev);
>> Â}
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index e444f5b..b41d9a1 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -281,6 +281,7 @@ struct pci_dev {
>>    unsigned int  Âno_d1d2:1;   Â/* Only allow D0 and D3 */
>>    unsigned int  Âmmio_always_on:1;    /* disallow turning off io/mem
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdecoding during bar sizing */
>> +   unsigned int  Âruntime_d3cold:1;
>>    unsigned int  Âwakeup_prepared:1;
>>    unsigned int  Âd3_delay;    /* D3->D0 transition time in ms */
>
> OK
>
> So now please tell me what exactly you want to achieve and why you want to do
> that in the first place.

Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/