Re: [RFC v2 5/5] PCIe, Add PCIe runtime D3cold support

From: huang ying
Date: Sat May 05 2012 - 04:08:18 EST

Next message: Sascha Hauer: "Re: Making ARM multiplatform kernels DT-only?"
Previous message: Sam Ravnborg: "[PATCH 0/8] kbuild: use script for final link, move definitionsout of top-level Makefile"
In reply to: Rafael J. Wysocki: "Re: [RFC v2 5/5] PCIe, Add PCIe runtime D3cold support"
Next in thread: Rafael J. Wysocki: "Re: [RFC v2 5/5] PCIe, Add PCIe runtime D3cold support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, May 5, 2012 at 4:50 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> On Friday, May 04, 2012, Huang Ying wrote:
>> This patch adds runtime D3cold support to PCIe bus. ÂD3cold is the
>> deepest power saving state for PCIe devices. ÂWhere the main PCIe link
>> will be powered off totally, so before the PCIe main link is powered
>> on again, you can not access the device, even the configuration space,
>> which is still accessible in D3hot. ÂBecause the main PCIe link is not
>> available, the PCI PM registers can not be used to put device into/out
>> of D3cold state, that will be done by platform logic such as ACPI
>> _PR3.
>>
>> To support remote wake up in D3cold, aux power is supplied, and a
>> side-band pin: WAKE# is used to notify remote wake up event, the pin
>> is usually connected to platform logic such as ACPI GPE. ÂThis is
>> quite different with other power saving states, where remote wake up
>> is notified via PME message via the PCIe main link.
>>
>> PCIe devices in plug-in slot usually has no direct platform logic, for
>> example, there is usually no ACPI _PR3 for them. ÂThe D3cold support
>> for these devices can be done via the PCIe port connected to it. ÂWhen
>> power on/off the PCIe port, the corresponding PCIe devices are powered
>> on/off too. ÂAnd the remote wake up event from PCIe device will be
>> notified to the corresponding PCIe port.
>>
>> The PCI subsystem D3cold support and corresponding ACPI platform
>> support is implemented in the patch. ÂIncluding the support for PCIe
>> devices in plug-in slot.
>>
>> For more information about PCIe D3cold and corresponding ACPI support,
>> please refer to:
>>
>> - PCI Express Base Specification Revision 2.0
>> - Advanced Configuration and Power Interface Specification Revision 5.0
>>
>> Originally-by: Zheng Yan <zheng.z.yan@xxxxxxxxx>
>> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxx>
>> ---
>> Âdrivers/pci/pci-acpi.c Â Â Â Â | Â 32 +++++++++--
>> Âdrivers/pci/pci-driver.c Â Â Â | Â Â3 +
>> Âdrivers/pci/pci.c Â Â Â Â Â Â Â| Â115 +++++++++++++++++++++++++++++++++++++----
>> Âdrivers/pci/pci.h Â Â Â Â Â Â Â| Â Â1
>> Âdrivers/pci/pcie/portdrv_pci.c | Â 28 +++++++++
>> Âinclude/linux/pci.h Â Â Â Â Â Â| Â 12 +++-
>> Â6 files changed, 175 insertions(+), 16 deletions(-)
>>
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -48,6 +48,21 @@ static void pci_acpi_wake_dev(acpi_handl
>> Â Â Â if (event != ACPI_NOTIFY_DEVICE_WAKE || !pci_dev)
>> Â Â Â Â Â Â Â return;
>>
>> + Â Â if (pci_dev->current_state == PCI_D3cold) {
>> + Â Â Â Â Â Â unsigned int count = 0;
>> +
>> + Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â* Powering on bridge need to resume whole hierarchy,
>> + Â Â Â Â Â Â Â* just resume the children to avoid the bridge going
>> + Â Â Â Â Â Â Â* suspending as soon as resumed
>> + Â Â Â Â Â Â Â*/
>
> Don't you need to resume the bridge before you start walking the hierarchy
> below it?

When we resume the hierarchy below the bridge, its parent, the bridge,
will be resumed firstly. That is:

rpm_resume(child)
rpm_resume(parent)
->runtime_suspend(child)

>> + Â Â Â Â Â Â if (pci_dev->subordinate)
>> + Â Â Â Â Â Â Â Â Â Â count = pci_wakeup_bus(pci_dev->subordinate);
>> + Â Â Â Â Â Â if (count == 0)
>> + Â Â Â Â Â Â Â Â Â Â pm_runtime_resume(&pci_dev->dev);
>
> What's the count for, exactly?

If there is no devices under the bridge, count returned will be 0,
then we will resume bridge itself.

>> + Â Â Â Â Â Â return;
>> + Â Â }
>> +
>> Â Â Â if (!pci_dev->pm_cap || !pci_dev->pme_support
>> Â Â Â Â Â Â|| pci_check_pme_status(pci_dev)) {
>> Â Â Â Â Â Â Â if (pci_dev->pme_poll)
>> @@ -187,10 +202,13 @@ acpi_status pci_acpi_remove_pm_notifier(
>>
>> Âstatic pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>> Â{
>> - Â Â int acpi_state;
>> + Â Â int acpi_state, d_max;
>>
>> - Â Â acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL,
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ACPI_STATE_D3);
>> + Â Â if (pdev->dev.power.power_must_be_on)
>> + Â Â Â Â Â Â d_max = ACPI_STATE_D3_HOT;
>> + Â Â else
>> + Â Â Â Â Â Â d_max = ACPI_STATE_D3_COLD;
>> + Â Â acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL, d_max);
>> Â Â Â if (acpi_state < 0)
>> Â Â Â Â Â Â Â return PCI_POWER_ERROR;
>>
>> @@ -297,7 +315,13 @@ static void acpi_pci_propagate_run_wake(
>>
>> Âstatic int acpi_pci_run_wake(struct pci_dev *dev, bool enable)
>> Â{
>> - Â Â if (dev->pme_interrupt)
>> + Â Â /*
>> + Â Â Â* Per PCI Express Base Specification Revision 2.0 section
>> + Â Â Â* 5.3.3.2 Link Wakeup, platform support is needed for D3cold
>> + Â Â Â* waking up to power on the main link even if there is PME
>> + Â Â Â* support for D3cold
>> + Â Â Â*/
>> + Â Â if (dev->pme_interrupt && !dev->runtime_d3cold)
>> Â Â Â Â Â Â Â return 0;
>>
>> Â Â Â if (!acpi_pm_device_run_wake(&dev->dev, enable))
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -1013,10 +1013,13 @@ static int pci_pm_runtime_suspend(struct
>> Â Â Â if (!pm || !pm->runtime_suspend)
>> Â Â Â Â Â Â Â return -ENOSYS;
>>
>> + Â Â dev->power.power_must_be_on = false;
>> Â Â Â error = pm->runtime_suspend(dev);
>> Â Â Â suspend_report_result(pm->runtime_suspend, error);
>> Â Â Â if (error)
>> Â Â Â Â Â Â Â return error;
>> + Â Â if (!dev->power.power_off_user)
>> + Â Â Â Â Â Â dev->power.power_must_be_on = true;
>
> That doesn't look good. ÂThe flag itself should be exported via sysfs, but
> only if it is known that power can be removed from the device.
>
>>
>> Â Â Â pci_fixup_device(pci_fixup_suspend, pci_dev);
>>
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -653,6 +653,12 @@ void pci_update_current_state(struct pci
>> Â Â Â if (dev->pm_cap) {
>> Â Â Â Â Â Â Â u16 pmcsr;
>>
>> + Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â* Configuration space is not accessible for device in
>> + Â Â Â Â Â Â Â* D3cold, so keep in D3cold for safety
>> + Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â if (dev->current_state == PCI_D3cold)
>> + Â Â Â Â Â Â Â Â Â Â return;
>> Â Â Â Â Â Â Â pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>> Â Â Â Â Â Â Â dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
>> Â Â Â } else {
>> @@ -671,8 +677,11 @@ static int pci_platform_power_transition
>>
>> Â Â Â if (platform_pci_power_manageable(dev)) {
>> Â Â Â Â Â Â Â error = platform_pci_set_power_state(dev, state);
>> - Â Â Â Â Â Â if (!error)
>> + Â Â Â Â Â Â if (!error) {
>> + Â Â Â Â Â Â Â Â Â Â if (state == PCI_D3cold)
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â dev->current_state = PCI_D3cold;
>
> + Â Â Â Â Â Â Â Â Â Â Â else ?

OK. Will add else here.

>> Â Â Â Â Â Â Â Â Â Â Â pci_update_current_state(dev, state);
>>
>> + Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â /* Fall back to PCI_D0 if native PM is not supported */
>> Â Â Â Â Â Â Â if (!dev->pm_cap)
>> Â Â Â Â Â Â Â Â Â Â Â dev->current_state = PCI_D0;
>> @@ -693,8 +702,49 @@ static int pci_platform_power_transition
>> Â */
>> Âstatic void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
>> Â{
>> - Â Â if (state == PCI_D0)
>> + Â Â if (state == PCI_D0) {
>> Â Â Â Â Â Â Â pci_platform_power_transition(dev, PCI_D0);
>> + Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â* Mandatory power management transition delays, see
>> + Â Â Â Â Â Â Â* PCI Express Base Specification Revision 2.0 Section
>> + Â Â Â Â Â Â Â* 6.6.1: Conventional Reset. ÂDo not delay for
>> + Â Â Â Â Â Â Â* devices powered on/off by corresponding bridge,
>> + Â Â Â Â Â Â Â* because have already delayed for the bridge.
>> + Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â if (dev->runtime_d3cold) {
>> + Â Â Â Â Â Â Â Â Â Â msleep(dev->d3cold_delay);
>> + Â Â Â Â Â Â Â Â Â Â /* When powering on a bridge from D3cold, the
>> + Â Â Â Â Â Â Â Â Â Â Â* whole hierarchy may be powered on into
>> + Â Â Â Â Â Â Â Â Â Â Â* D0uninitialized state, resume them to give
>> + Â Â Â Â Â Â Â Â Â Â Â* them a chance to suspend again */
>> + Â Â Â Â Â Â Â Â Â Â if (dev->subordinate)
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â pci_wakeup_bus(dev->subordinate);
>> + Â Â Â Â Â Â }
>> + Â Â }
>> +}
>> +
>> +/**
>> + * __pci_dev_set_current_state - Set current state of a PCI device
>> + * @dev: Device to handle
>> + * @data: pointer to state to be set
>> + */
>> +static int __pci_dev_set_current_state(struct pci_dev *dev, void *data)
>> +{
>> + Â Â pci_power_t state = *(pci_power_t *)data;
>> +
>> + Â Â dev->current_state = state;
>> + Â Â return 0;
>> +}
>> +
>> +/**
>> + * __pci_bus_set_current_state - Walk given bus and set current state of devices
>> + * @bus: Top bus of the subtree to walk.
>> + * @state: state to be set
>> + */
>> +static void __pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state)
>> +{
>> + Â Â if (bus)
>> + Â Â Â Â Â Â pci_walk_bus(bus, __pci_dev_set_current_state, &state);
>> Â}
>>
>> Â/**
>> @@ -706,8 +756,17 @@ static void __pci_start_power_transition
>> Â */
>> Âint __pci_complete_power_transition(struct pci_dev *dev, pci_power_t state)
>> Â{
>> - Â Â return state >= PCI_D0 ?
>> - Â Â Â Â Â Â Â Â Â Â pci_platform_power_transition(dev, state) : -EINVAL;
>> + Â Â int ret;
>> +
>> + Â Â if (state < PCI_D0)
>> + Â Â Â Â Â Â return -EINVAL;
>> + Â Â ret = pci_platform_power_transition(dev, state);
>> + Â Â if (!ret && state == PCI_D3cold) {
>> + Â Â Â Â Â Â /* Power off the bridge may power off the whole hierarchy */
>> + Â Â Â Â Â Â if (dev->subordinate)
>> + Â Â Â Â Â Â Â Â Â Â __pci_bus_set_current_state(dev->subordinate, state);
>
> I'd just say
>
> + Â Â Â Â Â Â Â Â Â Â Â __pci_bus_set_current_state(dev->subordinate, PCI_D3cold);
>
> here. ÂAlso you don't need the if (dev->subordinate) check, because
> __pci_bus_set_current_state() will do it anyway.

OK. Will do that.

>> + Â Â }
>> + Â Â return ret;
>> Â}
>> ÂEXPORT_SYMBOL_GPL(__pci_complete_power_transition);
>>
>> @@ -731,8 +790,8 @@ int pci_set_power_state(struct pci_dev *
>> Â Â Â int error;
>>
>> Â Â Â /* 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))
>> @@ -747,10 +806,15 @@ int pci_set_power_state(struct pci_dev *
>>
>> Â Â Â /* This device is quirked not to be put into D3, so
>> Â Â Â Â Âdon't put it in D3 */
>> - Â Â if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
>> + Â Â if (state >= PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
>> Â Â Â Â Â Â Â return 0;
>>
>> - Â Â error = pci_raw_set_power_state(dev, state);
>> + Â Â /*
>> + Â Â Â* To put device in D3cold, we put device into D3hot in native
>> + Â Â Â* way, then put device into D3cold with platform ops
>> + Â Â Â*/
>> + Â Â error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â PCI_D3hot : state);
>>
>> Â Â Â if (!__pci_complete_power_transition(dev, state))
>> Â Â Â Â Â Â Â error = 0;
>> @@ -1506,6 +1570,34 @@ void pci_pme_wakeup_bus(struct pci_bus *
>> Â}
>>
>> Â/**
>> + * pci_wakeup - Wake up a PCI device
>> + * @dev: Device to handle.
>> + * @data: to count how many device are waken up.
>> + */
>> +static int pci_wakeup(struct pci_dev *dev, void *data)
>> +{
>> + Â Â unsigned int *count = data;
>> +
>> + Â Â pci_wakeup_event(dev);
>> + Â Â pm_request_resume(&dev->dev);
>> + Â Â (*count)++;
>> + Â Â return 0;
>> +}
>> +
>> +/**
>> + * pci_wakeup_bus - Walk given bus and wake up devices on it
>> + * @bus: Top bus of the subtree to walk.
>> + */
>> +unsigned int pci_wakeup_bus(struct pci_bus *bus)
>> +{
>> + Â Â unsigned int count = 0;
>> +
>> + Â Â if (bus)
>> + Â Â Â Â Â Â pci_walk_bus(bus, pci_wakeup, &count);
>> + Â Â return count;
>> +}
>> +
>> +/**
>> Â * pci_pme_capable - check the capability of PCI device to generate PME#
>> Â * @dev: PCI device to handle.
>> Â * @state: PCI state from which device will issue PME#.
>> @@ -1789,12 +1881,16 @@ int pci_finish_runtime_suspend(struct pc
>> Â Â Â if (target_state == PCI_POWER_ERROR)
>> Â Â Â Â Â Â Â return -EIO;
>>
>> + Â Â dev->runtime_d3cold = target_state == PCI_D3cold ? true : false;
>
> + Â Â Â dev->runtime_d3cold = target_state == PCI_D3cold;
>
> will suffice.

OK. Will do that.

>> +
>> Â Â Â __pci_enable_wake(dev, target_state, true, pci_dev_run_wake(dev));
>>
>> Â Â Â error = pci_set_power_state(dev, target_state);
>>
>> - Â Â if (error)
>> + Â Â if (error) {
>> Â Â Â Â Â Â Â __pci_enable_wake(dev, target_state, true, false);
>> + Â Â Â Â Â Â dev->runtime_d3cold = false;
>> + Â Â }
>>
>> Â Â Â return error;
>> Â}
>> @@ -1864,6 +1960,7 @@ void pci_pm_init(struct pci_dev *dev)
>>
>> Â Â Â dev->pm_cap = pm;
>> Â Â Â dev->d3_delay = PCI_PM_D3_WAIT;
>> + Â Â dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>>
>> Â Â Â dev->d1_support = false;
>> Â Â Â dev->d2_support = false;
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -70,6 +70,7 @@ extern void pci_update_current_state(str
>> Âextern void pci_disable_enabled_device(struct pci_dev *dev);
>> Âextern int pci_finish_runtime_suspend(struct pci_dev *dev);
>> Âextern int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
>> +extern unsigned int pci_wakeup_bus(struct pci_bus *bus);
>> Âextern void pci_pm_init(struct pci_dev *dev);
>> Âextern void platform_pci_wakeup_init(struct pci_dev *dev);
>> Âextern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -101,10 +101,38 @@ static int pcie_port_resume_noirq(struct
>> Â}
>>
>> Â#ifdef CONFIG_PM_RUNTIME
>> +struct d3cold_info {
>> + Â Â bool power_must_be_on;
>> + Â Â unsigned int d3cold_delay;
>> +};
>> +
>> +static int pci_dev_d3cold_info(struct pci_dev *pdev, void *data)
>> +{
>> + Â Â struct d3cold_info *info = data;
>> +
>> + Â Â info->d3cold_delay = max_t(unsigned int, pdev->d3cold_delay,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âinfo->d3cold_delay);
>> + Â Â info->power_must_be_on = info->power_must_be_on ||
>> + Â Â Â Â Â Â pdev->dev.power.power_must_be_on;
>> + Â Â return 0;
>> +}
>> +
>> Âstatic int pcie_port_runtime_suspend(struct device *dev)
>> Â{
>> Â Â Â struct pci_dev *pdev = to_pci_dev(dev);
>> + Â Â struct d3cold_info d3cold_info = {
>> + Â Â Â Â Â Â .power_must_be_on Â Â Â = dev->power.power_must_be_on,
>> + Â Â Â Â Â Â .d3cold_delay Â Â Â Â Â = PCI_PM_D3_WAIT,
>> + Â Â };
>>
>> + Â Â /*
>> + Â Â Â* If any subordinate device need to keep power on, we should
>> + Â Â Â* not power off the port. ÂThe D3cold delay of port should be
>> + Â Â Â* the max of that of all subordinate devices.
>
> What if some of the devices below the port are ports themselves? ÂOr PCI-to-PCIe
> bridges?

For them, I think the current solution is safe.

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/

Next message: Sascha Hauer: "Re: Making ARM multiplatform kernels DT-only?"
Previous message: Sam Ravnborg: "[PATCH 0/8] kbuild: use script for final link, move definitionsout of top-level Makefile"
In reply to: Rafael J. Wysocki: "Re: [RFC v2 5/5] PCIe, Add PCIe runtime D3cold support"
Next in thread: Rafael J. Wysocki: "Re: [RFC v2 5/5] PCIe, Add PCIe runtime D3cold support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]