On Thursday 23 April 2009, Alan Stern wrote:
> Rafael:
Hi,
> This patch converts the USB PCI-based host controller drivers over to
> the new PM framework. Does it all look right to you?
Yes, it does, from a quick look, but I've always wondered why you insist on
putting the devices into D3. Is it a result of the experimental observation
that none of the other states work in general?
In principle ACPI might want to put them into D1 or D2 before suspend,
especially is wake-up is enabled.
Thanks,
Rafael
> Index: usb-2.6/drivers/usb/core/hcd.h
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hcd.h
> +++ usb-2.6/drivers/usb/core/hcd.h
> @@ -261,14 +261,11 @@ struct pci_device_id;
> extern int usb_hcd_pci_probe(struct pci_dev *dev,
> const struct pci_device_id *id);
> extern void usb_hcd_pci_remove(struct pci_dev *dev);
> -
> -#ifdef CONFIG_PM
> -extern int usb_hcd_pci_suspend(struct pci_dev *dev, pm_message_t msg);
> -extern int usb_hcd_pci_resume(struct pci_dev *dev);
> -#endif /* CONFIG_PM */
> -
> extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
>
> +#ifdef CONFIG_PM_SLEEP
> +extern struct dev_pm_ops usb_hcd_pci_pm_ops;
> +#endif
> #endif /* CONFIG_PCI */
>
> /* pci-ish (pdev null is ok) buffer alloc/mapping support */
> Index: usb-2.6/drivers/usb/core/hcd-pci.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hcd-pci.c
> +++ usb-2.6/drivers/usb/core/hcd-pci.c
> @@ -185,194 +185,207 @@ void usb_hcd_pci_remove(struct pci_dev *
> }
> EXPORT_SYMBOL_GPL(usb_hcd_pci_remove);
>
> -
> -#ifdef CONFIG_PM
> -
> /**
> - * usb_hcd_pci_suspend - power management suspend of a PCI-based HCD
> - * @dev: USB Host Controller being suspended
> - * @message: Power Management message describing this state transition
> - *
> - * Store this function in the HCD's struct pci_driver as .suspend.
> + * usb_hcd_pci_shutdown - shutdown host controller
> + * @dev: USB Host Controller being shutdown
> */
> -int usb_hcd_pci_suspend(struct pci_dev *dev, pm_message_t message)
> +void usb_hcd_pci_shutdown(struct pci_dev *dev)
> {
> - struct usb_hcd *hcd = pci_get_drvdata(dev);
> - int retval = 0;
> - int wake, w;
> - int has_pci_pm;
> + struct usb_hcd *hcd;
> +
> + hcd = pci_get_drvdata(dev);
> + if (!hcd)
> + return;
> +
> + if (hcd->driver->shutdown)
> + hcd->driver->shutdown(hcd);
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_pci_shutdown);
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +static int check_root_hub_suspended(struct device *dev)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
> +
> + if (!(hcd->state == HC_STATE_SUSPENDED ||
> + hcd->state == HC_STATE_HALT)) {
> + dev_warn(dev, "Root hub is not suspended\n");
> + return -EBUSY;
> + }
> + return 0;
> +}
> +
> +static int hcd_pci_suspend(struct device *dev)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
> + int retval;
>
> /* Root hub suspend should have stopped all downstream traffic,
> * and all bus master traffic. And done so for both the interface
> * and the stub usb_device (which we check here). But maybe it
> * didn't; writing sysfs power/state files ignores such rules...
> - *
> - * We must ignore the FREEZE vs SUSPEND distinction here, because
> - * otherwise the swsusp will save (and restore) garbage state.
> */
> - if (!(hcd->state == HC_STATE_SUSPENDED ||
> - hcd->state == HC_STATE_HALT)) {
> - dev_warn(&dev->dev, "Root hub is not suspended\n");
> - retval = -EBUSY;
> - goto done;
> - }
> + retval = check_root_hub_suspended(dev);
> + if (retval)
> + return retval;
>
> /* We might already be suspended (runtime PM -- not yet written) */
> - if (dev->current_state != PCI_D0)
> - goto done;
> + if (pci_dev->current_state != PCI_D0)
> + return retval;
>
> if (hcd->driver->pci_suspend) {
> - retval = hcd->driver->pci_suspend(hcd, message);
> + retval = hcd->driver->pci_suspend(hcd, PMSG_SUSPEND);
> suspend_report_result(hcd->driver->pci_suspend, retval);
> if (retval)
> - goto done;
> + return retval;
> }
>
> - synchronize_irq(dev->irq);
> + synchronize_irq(pci_dev->irq);
>
> /* Downstream ports from this root hub should already be quiesced, so
> * there will be no DMA activity. Now we can shut down the upstream
> - * link (except maybe for PME# resume signaling) and enter some PCI
> - * low power state, if the hardware allows.
> + * link (except maybe for PME# resume signaling). We'll enter a
> + * low power state during suspend_noirq, if the hardware allows.
> */
> - pci_disable_device(dev);
> + pci_disable_device(pci_dev);
> + return retval;
> +}
> +
> +static int hcd_pci_suspend_noirq(struct device *dev)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
> + int retval;
> + int wake, w;
>
> - pci_save_state(dev);
> + retval = check_root_hub_suspended(dev);
> + if (retval)
> + return retval;
> +
> + pci_save_state(pci_dev);
>
> /* Don't fail on error to enable wakeup. We rely on pci code
> * to reject requests the hardware can't implement, rather
> * than coding the same thing.
> */
> - wake = (hcd->state == HC_STATE_SUSPENDED &&
> - device_may_wakeup(&dev->dev));
> - w = pci_wake_from_d3(dev, wake);
> + wake = (hcd->state == HC_STATE_SUSPENDED && device_may_wakeup(dev));
> + w = pci_wake_from_d3(pci_dev, wake);
> if (w < 0)
> wake = w;
> - dev_dbg(&dev->dev, "wakeup: %d\n", wake);
> + dev_dbg(dev, "wakeup: %d\n", wake);
>
> - /* Don't change state if we don't need to */
> - if (message.event == PM_EVENT_FREEZE ||
> - message.event == PM_EVENT_PRETHAW) {
> - dev_dbg(&dev->dev, "--> no state change\n");
> - goto done;
> - }
> -
> - has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
> - if (!has_pci_pm) {
> - dev_dbg(&dev->dev, "--> PCI D0 legacy\n");
> + /* NOTE: pci_dev->current_state becomes nonzero only here,
> + * and only for devices that support PCI PM. Also, exiting
> + * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
> + * some device state (e.g. as part of clock reinit).
> + */
> + retval = pci_set_power_state(pci_dev, PCI_D3hot);
> + if (retval == -EIO) { /* D3 not supported */
> + dev_dbg(dev, "--> PCI D0 legacy\n");
> + retval = 0;
> + } else if (retval == 0) {
> + dev_dbg(dev, "--> PCI D3\n");
> } else {
> -
> - /* NOTE: dev->current_state becomes nonzero only here, and
> - * only for devices that support PCI PM. Also, exiting
> - * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
> - * some device state (e.g. as part of clock reinit).
> - */
> - retval = pci_set_power_state(dev, PCI_D3hot);
> suspend_report_result(pci_set_power_state, retval);
> - if (retval == 0) {
> - dev_dbg(&dev->dev, "--> PCI D3\n");
> - } else {
> - dev_dbg(&dev->dev, "PCI D3 suspend fail, %d\n",
> - retval);
> - pci_restore_state(dev);
> - }
> + pci_wake_from_d3(pci_dev, false);
> + return retval;
> }
>
> #ifdef CONFIG_PPC_PMAC
> - if (retval == 0) {
> - /* Disable ASIC clocks for USB */
> - if (machine_is(powermac)) {
> - struct device_node *of_node;
> -
> - of_node = pci_device_to_OF_node(dev);
> - if (of_node)
> - pmac_call_feature(PMAC_FTR_USB_ENABLE,
> - of_node, 0, 0);
> - }
> + /* Disable ASIC clocks for USB */
> + if (machine_is(powermac)) {
> + struct device_node *of_node;
> +
> + of_node = pci_device_to_OF_node(pci_dev);
> + if (of_node)
> + pmac_call_feature(PMAC_FTR_USB_ENABLE, of_node, 0, 0);
> }
> #endif
> -
> - done:
> return retval;
> }
> -EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend);
>
> -/**
> - * usb_hcd_pci_resume - power management resume of a PCI-based HCD
> - * @dev: USB Host Controller being resumed
> - *
> - * Store this function in the HCD's struct pci_driver as .resume.
> - */
> -int usb_hcd_pci_resume(struct pci_dev *dev)
> +static int hcd_pci_resume_noirq(struct device *dev)
> {
> - struct usb_hcd *hcd;
> - int retval;
> + struct pci_dev *pci_dev = to_pci_dev(dev);
>
> #ifdef CONFIG_PPC_PMAC
> /* Reenable ASIC clocks for USB */
> if (machine_is(powermac)) {
> struct device_node *of_node;
>
> - of_node = pci_device_to_OF_node(dev);
> + of_node = pci_device_to_OF_node(pci_dev);
> if (of_node)
> pmac_call_feature(PMAC_FTR_USB_ENABLE,
> of_node, 0, 1);
> }
> #endif
>
> - pci_restore_state(dev);
> + /* We don't have to set the power state back to D0 because
> + * the PCI core has done so already.
> + */
> +
> + pci_wake_from_d3(pci_dev, false);
> + return 0;
> +}
> +
> +static int resume_common(struct device *dev, bool hibernated)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
> + int retval;
>
> - hcd = pci_get_drvdata(dev);
> if (hcd->state != HC_STATE_SUSPENDED) {
> - dev_dbg(hcd->self.controller,
> - "can't resume, not suspended!\n");
> + dev_dbg(dev, "can't resume, not suspended!\n");
> return 0;
> }
>
> - pci_enable_wake(dev, PCI_D0, false);
> -
> - retval = pci_enable_device(dev);
> + retval = pci_enable_device(pci_dev);
> if (retval < 0) {
> - dev_err(&dev->dev, "can't re-enable after resume, %d!\n",
> - retval);
> + dev_err(dev, "can't re-enable after resume, %d!\n", retval);
> return retval;
> }
>
> - pci_set_master(dev);
> -
> - /* yes, ignore this result too... */
> - (void) pci_wake_from_d3(dev, 0);
> + pci_set_master(pci_dev);
>
> clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
>
> if (hcd->driver->pci_resume) {
> retval = hcd->driver->pci_resume(hcd);
> if (retval) {
> - dev_err(hcd->self.controller,
> - "PCI post-resume error %d!\n", retval);
> + dev_err(dev, "PCI post-resume error %d!\n", retval);
> usb_hc_died(hcd);
> }
> }
> return retval;
> }
> -EXPORT_SYMBOL_GPL(usb_hcd_pci_resume);
>
> -#endif /* CONFIG_PM */
> -
> -/**
> - * usb_hcd_pci_shutdown - shutdown host controller
> - * @dev: USB Host Controller being shutdown
> - */
> -void usb_hcd_pci_shutdown(struct pci_dev *dev)
> +static int hcd_pci_resume(struct device *dev)
> {
> - struct usb_hcd *hcd;
> -
> - hcd = pci_get_drvdata(dev);
> - if (!hcd)
> - return;
> + return resume_common(dev, false);
> +}
>
> - if (hcd->driver->shutdown)
> - hcd->driver->shutdown(hcd);
> +static int hcd_pci_restore(struct device *dev)
> +{
> + return resume_common(dev, true);
> }
> -EXPORT_SYMBOL_GPL(usb_hcd_pci_shutdown);
>
> +struct dev_pm_ops usb_hcd_pci_pm_ops = {
> + .suspend = hcd_pci_suspend,
> + .suspend_noirq = hcd_pci_suspend_noirq,
> + .resume_noirq = hcd_pci_resume_noirq,
> + .resume = hcd_pci_resume,
> + .freeze = check_root_hub_suspended,
> + .freeze_noirq = check_root_hub_suspended,
> + .thaw_noirq = NULL,
> + .thaw = NULL,
> + .poweroff = hcd_pci_suspend,
> + .poweroff_noirq = hcd_pci_suspend_noirq,
> + .restore_noirq = hcd_pci_resume_noirq,
> + .restore = hcd_pci_restore,
> +};
> +EXPORT_SYMBOL_GPL(usb_hcd_pci_pm_ops);
> +
> +#endif /* CONFIG_PM_SLEEP */
> Index: usb-2.6/drivers/usb/host/ehci-pci.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-pci.c
> +++ usb-2.6/drivers/usb/host/ehci-pci.c
> @@ -429,10 +429,11 @@ static struct pci_driver ehci_pci_driver
>
> .probe = usb_hcd_pci_probe,
> .remove = usb_hcd_pci_remove,
> + .shutdown = usb_hcd_pci_shutdown,
>
> -#ifdef CONFIG_PM
> - .suspend = usb_hcd_pci_suspend,
> - .resume = usb_hcd_pci_resume,
> +#ifdef CONFIG_PM_SLEEP
> + .driver = {
> + .pm = &usb_hcd_pci_pm_ops
> + },
> #endif
> - .shutdown = usb_hcd_pci_shutdown,
> };
> Index: usb-2.6/drivers/usb/host/ohci-pci.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ohci-pci.c
> +++ usb-2.6/drivers/usb/host/ohci-pci.c
> @@ -484,12 +484,11 @@ static struct pci_driver ohci_pci_driver
>
> .probe = usb_hcd_pci_probe,
> .remove = usb_hcd_pci_remove,
> + .shutdown = usb_hcd_pci_shutdown,
>
> -#ifdef CONFIG_PM
> - .suspend = usb_hcd_pci_suspend,
> - .resume = usb_hcd_pci_resume,
> +#ifdef CONFIG_PM_SLEEP
> + .driver = {
> + .pm = &usb_hcd_pci_pm_ops
> + },
> #endif
> -
> - .shutdown = usb_hcd_pci_shutdown,
> };
> -
> Index: usb-2.6/drivers/usb/host/uhci-hcd.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/uhci-hcd.c
> +++ usb-2.6/drivers/usb/host/uhci-hcd.c
> @@ -940,10 +940,11 @@ static struct pci_driver uhci_pci_driver
> .remove = usb_hcd_pci_remove,
> .shutdown = uhci_shutdown,
>
> -#ifdef CONFIG_PM
> - .suspend = usb_hcd_pci_suspend,
> - .resume = usb_hcd_pci_resume,
> -#endif /* PM */
> +#ifdef CONFIG_PM_SLEEP
> + .driver = {
> + .pm = &usb_hcd_pci_pm_ops
> + },
> +#endif
> };
>
> static int __init uhci_hcd_init(void)
>
>
>
--
Everyone knows that debugging is twice as hard as writing a program
in the first place. So if you're as clever as you can be when you write it,
how will you ever debug it? --- Brian Kernighan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
