lkml.org 
[lkml]   [2012]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC v2 3/5] PCIe, Add runtime PM support to PCIe port
    From
    On Sat, May 5, 2012 at 3:50 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
    > On Fri, May 4, 2012 at 2:13 AM, Huang Ying <ying.huang@intel.com> wrote:
    >> From: Zheng Yan <zheng.z.yan@intel.com>
    >>
    >> This patch adds runtime PM support to PCIe port.  This is needed by
    >> PCIe D3cold support, where PCIe device in slot may be powered on/off
    >> by PCIe port.
    >
    > I assume this works for integrated PCIe devices as well as those that
    > are plugged into a slot and can be physically removed -- maybe the
    > text "in slot" is superfluous?

    "in slot" is used here, because usually integrated PCIe devices has
    ACPI node and method attached with them while that is not the case for
    PCIe devices in slot. I will make it more clear with something as
    follow:

    Some PCIe devices have not direct platform support (such as ACPI node
    and method) to be powered on/off. But they may be powered on/off by
    the corresponding PCIe port.

    >> Because runtime suspend is broken for some chipset, a white list is
    >> used to enable runtime PM support for only chipset known to work.
    >
    > A whitelist requires perpetual maintenance.  Every time a new working
    > chipset comes out, you have to update the whitelist.  That doesn't
    > seem right.
    >
    >> Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
    >> Signed-off-by: Huang Ying <ying.huang@intel.com>
    >> ---
    >>  drivers/pci/pci.c              |    9 +++++++++
    >>  drivers/pci/pcie/portdrv_pci.c |   40 ++++++++++++++++++++++++++++++++++++++++
    >>  2 files changed, 49 insertions(+)
    >>
    >> --- a/drivers/pci/pci.c
    >> +++ b/drivers/pci/pci.c
    >> @@ -1476,6 +1476,15 @@ bool pci_check_pme_status(struct pci_dev
    >>  */
    >>  static int pci_pme_wakeup(struct pci_dev *dev, void *pme_poll_reset)
    >>  {
    >> +       struct pci_dev *bridge = dev->bus->self;
    >> +
    >> +       /*
    >> +        * If bridge is in low power state, the configuration space of
    >> +        * subordinate devices may be not accessible
    >> +        */
    >
    > This comment would make more sense as "If the upstream bridge is in a
    > low power state, the configuration space of this device is not
    > accessible."

    Yes. Will change this.

    >> +       if (bridge && bridge->current_state != PCI_D0)
    >> +               return 0;
    >> +
    >>        if (pme_poll_reset && dev->pme_poll)
    >>                dev->pme_poll = false;
    >>
    >> --- a/drivers/pci/pcie/portdrv_pci.c
    >> +++ b/drivers/pci/pcie/portdrv_pci.c
    >> @@ -11,6 +11,7 @@
    >>  #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>
    >> @@ -99,6 +100,27 @@ static int pcie_port_resume_noirq(struct
    >>        return 0;
    >>  }
    >>
    >> +#ifdef CONFIG_PM_RUNTIME
    >> +static int pcie_port_runtime_suspend(struct device *dev)
    >> +{
    >> +       struct pci_dev *pdev = to_pci_dev(dev);
    >> +
    >> +       pci_save_state(pdev);
    >> +       return 0;
    >> +}
    >> +
    >> +static int pcie_port_runtime_resume(struct device *dev)
    >> +{
    >> +       struct pci_dev *pdev = to_pci_dev(dev);
    >> +
    >> +       pci_restore_state(pdev);
    >> +       return 0;
    >> +}
    >> +#else
    >> +#define pcie_port_runtime_suspend      NULL
    >> +#define pcie_port_runtime_resume       NULL
    >> +#endif
    >> +
    >>  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
    >>        .suspend        = pcie_port_device_suspend,
    >>        .resume         = pcie_port_device_resume,
    >> @@ -107,6 +129,8 @@ static const struct dev_pm_ops pcie_port
    >>        .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)
    >> @@ -117,6 +141,18 @@ static const struct dev_pm_ops pcie_port
    >>  #endif /* !PM */
    >>
    >>  /*
    >> + * PCIe port runtime suspend is broken for some chipset, so use a
    >> + * white list to disable runtime PM for these chipset.
    >
    > If you're *disabling* runtime PM for these chipsets, I'd call this a
    > blacklist (and a blacklist makes more sense to me from a maintenance
    > standpoint, because you only have to update it when new broken chips
    > are discovered).

    Yes. I think blacklist may be better.

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

    \
     
     \ /
      Last update: 2012-05-05 09:21    [W:5.500 / U:0.452 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site