On Fri, 30 Apr 2010 01:45:33 +0800
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> This patch fixes the problem on my system. Does it work for you?
> I still think that disabling wakeup at the PCI or platform level should
> override the port wakeup flags, but apparently it doesn't.
>
> Alek, can you confirm that the changes in this patch are okay for the
> Moorestown EHCI controller? I had to guess at how to handle the
> HOSTPCx register settings.
>
> Alan Stern
Alan,
As I tested, the patch breaks phy low power mode, the EHCI works, but it never
enter phy low power mode when suspending port...
I guess this part of the diff breaks it.
- /* only enable appropriate wake bits, otherwise the
- * hardware can not go phy low power mode. If a race
- * condition happens here(connection change during bits
- * set), the port change detection will finally fix it.
- */
- if (t1 & PORT_CONNECT) {
- t2 |= PORT_WKOC_E | PORT_WKDISC_E;
- t2 &= ~PORT_WKCONN_E;
- } else {
- t2 |= PORT_WKOC_E | PORT_WKCONN_E;
- t2 &= ~PORT_WKDISC_E;
- }
Thanks,
Alek
>
>
>
> Index: usb-2.6/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-hub.c
> +++ usb-2.6/drivers/usb/host/ehci-hub.c
> @@ -106,6 +106,47 @@ static void ehci_handover_companion_port
> ehci->owned_ports = 0;
> }
>
> +static void ehci_set_wakeup_flags(struct ehci_hcd *ehci)
> +{
> + int port;
> +
> + /* enable remote wakeup on all ports, if allowed */
> + port = HCS_N_PORTS(ehci->hcs_params);
> + while (port--) {
> + u32 __iomem *reg = &ehci->regs->port_status[port];
> + u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
> + u32 t2 = t1 & ~PORT_WAKE_BITS;
> +
> + /* only enable appropriate wake bits, otherwise the
> + * hardware can not go phy low power mode. If a race
> + * condition happens here(connection change during bits
> + * set), the port change detection will finally fix it.
> + */
> + if (device_may_wakeup(ehci_to_hcd(ehci)->self.controller)) {
> + if (t1 & PORT_CONNECT)
> + t2 |= PORT_WKOC_E | PORT_WKDISC_E;
> + else
> + t2 |= PORT_WKOC_E | PORT_WKCONN_E;
> + ehci_vdbg(ehci, "port %d, %08x -> %08x\n",
> + port + 1, t1, t2);
> + }
> + ehci_writel(ehci, t2, reg);
> + }
> +}
> +
> +static void ehci_clear_wakeup_flags(struct ehci_hcd *ehci)
> +{
> + int port;
> +
> + port = HCS_N_PORTS(ehci->hcs_params);
> + while (port--) {
> + u32 __iomem *reg = &ehci->regs->port_status[port];
> + u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
> +
> + ehci_writel(ehci, t1 & ~PORT_WAKE_BITS, reg);
> + }
> +}
> +
> static int ehci_bus_suspend (struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> @@ -170,26 +211,6 @@ static int ehci_bus_suspend (struct usb_
> else if ((t1 & PORT_PE) && !(t1 & PORT_SUSPEND)) {
> t2 |= PORT_SUSPEND;
> set_bit(port, &ehci->bus_suspended);
> - }
> -
> - /* enable remote wakeup on all ports */
> - if (hcd->self.root_hub->do_remote_wakeup) {
> - /* only enable appropriate wake bits, otherwise the
> - * hardware can not go phy low power mode. If a race
> - * condition happens here(connection change during bits
> - * set), the port change detection will finally fix it.
> - */
> - if (t1 & PORT_CONNECT) {
> - t2 |= PORT_WKOC_E | PORT_WKDISC_E;
> - t2 &= ~PORT_WKCONN_E;
> - } else {
> - t2 |= PORT_WKOC_E | PORT_WKCONN_E;
> - t2 &= ~PORT_WKDISC_E;
> - }
> - } else
> - t2 &= ~PORT_WAKE_BITS;
> -
> - if (t1 != t2) {
> ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
> port + 1, t1, t2);
> ehci_writel(ehci, t2, reg);
> @@ -907,12 +928,7 @@ static int ehci_hub_control (
> || (temp & PORT_RESET) != 0)
> goto error;
>
> - /* After above check the port must be connected.
> - * Set appropriate bit thus could put phy into low power
> - * mode if we have hostpc feature
> - */
> - temp &= ~PORT_WKCONN_E;
> - temp |= PORT_WKDISC_E | PORT_WKOC_E;
> + temp &= ~PORT_WAKE_BITS;
> ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
> if (hostpc_reg) {
> spin_unlock_irqrestore(&ehci->lock, flags);
> 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
> @@ -284,23 +284,15 @@ static int ehci_pci_suspend(struct usb_h
> msleep(10);
>
> /* Root hub was already suspended. Disable irq emission and
> - * mark HW unaccessible, bail out if RH has been resumed. Use
> - * the spinlock to properly synchronize with possible pending
> - * RH suspend or resume activity.
> - *
> - * This is still racy as hcd->state is manipulated outside of
> - * any locks =P But that will be a different fix.
> + * mark HW unaccessible. The PM and USB cores make sure that
> + * the root hub is either suspended or stopped.
> */
> spin_lock_irqsave (&ehci->lock, flags);
> - if (hcd->state != HC_STATE_SUSPENDED) {
> - rc = -EINVAL;
> - goto bail;
> - }
> + ehci_set_wakeup_flags(ehci);
> ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> (void)ehci_readl(ehci, &ehci->regs->intr_enable);
>
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> - bail:
> spin_unlock_irqrestore (&ehci->lock, flags);
>
> // could save FLADJ in case of Vaux power loss
> @@ -330,6 +322,7 @@ static int ehci_pci_resume(struct usb_hc
> !hibernated) {
> int mask = INTR_MASK;
>
> + ehci_clear_wakeup_flags(ehci);
> if (!hcd->self.root_hub->do_remote_wakeup)
> mask &= ~STS_PCD;
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> Index: usb-2.6/drivers/usb/host/ehci-au1xxx.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-au1xxx.c
> +++ usb-2.6/drivers/usb/host/ehci-au1xxx.c
> @@ -215,26 +215,17 @@ static int ehci_hcd_au1xxx_drv_suspend(s
> msleep(10);
>
> /* Root hub was already suspended. Disable irq emission and
> - * mark HW unaccessible, bail out if RH has been resumed. Use
> - * the spinlock to properly synchronize with possible pending
> - * RH suspend or resume activity.
> - *
> - * This is still racy as hcd->state is manipulated outside of
> - * any locks =P But that will be a different fix.
> + * mark HW unaccessible. The PM and USB cores make sure that
> + * the root hub is either suspended or stopped.
> */
> spin_lock_irqsave(&ehci->lock, flags);
> - if (hcd->state != HC_STATE_SUSPENDED) {
> - rc = -EINVAL;
> - goto bail;
> - }
> + ehci_set_wakeup_flags(ehci);
> ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> (void)ehci_readl(ehci, &ehci->regs->intr_enable);
>
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>
> au1xxx_stop_ehc();
> -
> -bail:
> spin_unlock_irqrestore(&ehci->lock, flags);
>
> // could save FLADJ in case of Vaux power loss
> @@ -264,6 +255,7 @@ static int ehci_hcd_au1xxx_drv_resume(st
> if (ehci_readl(ehci, &ehci->regs->configured_flag) == FLAG_CF) {
> int mask = INTR_MASK;
>
> + ehci_clear_wakeup_flags(ehci);
> if (!hcd->self.root_hub->do_remote_wakeup)
> mask &= ~STS_PCD;
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> Index: usb-2.6/drivers/usb/host/ehci-fsl.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-2.6/drivers/usb/host/ehci-fsl.c
> @@ -313,6 +313,7 @@ static int ehci_fsl_drv_suspend(struct d
> struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> void __iomem *non_ehci = hcd->regs;
>
> + ehci_set_wakeup_flags(ehci);
> if (!fsl_deep_sleep())
> return 0;
>
> @@ -327,6 +328,7 @@ static int ehci_fsl_drv_resume(struct de
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> void __iomem *non_ehci = hcd->regs;
>
> + ehci_clear_wakeup_flags(ehci);
> if (!fsl_deep_sleep())
> return 0;
>
>
--
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
