On Thu, 11 Jun 2009, Alek Du wrote:
> From b6295cd7977ce3384dca7bd9a510e7571cc62496 Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@xxxxxxxxx>
> Date: Wed, 10 Jun 2009 14:38:15 +0800
> Subject: [PATCH] EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support low power mode
>
> The Intel Moorestown EHCI controller supports non-standard HOSTPCx register
> extension. This register controls the LPM behaviour and controls the behaviour
> of each USB port.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxx>
> Signed-off-by: Alek Du <alek.du@xxxxxxxxx>
> ---
> drivers/usb/host/ehci-hcd.c | 4 +++
> drivers/usb/host/ehci-hub.c | 59 +++++++++++++++++++++++++++++++++++++----
> drivers/usb/host/ehci.h | 9 ++++++
> include/linux/usb/ehci_def.h | 9 ++++++
> 4 files changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 36cedc9..a546ce6 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -249,6 +249,10 @@ static int ehci_reset (struct ehci_hcd *ehci)
> retval = handshake (ehci, &ehci->regs->command,
> CMD_RESET, 0, 250 * 1000);
>
> + if (ehci->has_hostpc) {
> + ehci_writel(ehci, 0x23, &ehci->regs->command + (0xc8/4));
> + ehci_writel(ehci, 0x80000, &ehci->regs->command + (0x24/4));
How about using symbolic constants instead of magic numbers?
> + }
> if (retval)
> return retval;
>
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 97a53a4..51be77a 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -111,6 +111,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> int port;
> int mask;
> + u32 __iomem *hostpc_reg = NULL;
>
> ehci_dbg(ehci, "suspend root hub\n");
>
> @@ -142,6 +143,9 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> u32 t1 = ehci_readl(ehci, reg) & ~PORT_RWC_BITS;
> u32 t2 = t1;
>
> + if (ehci->has_hostpc)
> + hostpc_reg = (u32 __iomem *)((unsigned char *)ehci->regs
> + + HOSTPC0 + 4 * (port & 0xff));
> /* keep track of which ports we suspend */
> if (t1 & PORT_OWNER)
> set_bit(port, &ehci->owned_ports);
> @@ -151,15 +155,31 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> }
>
> /* enable remote wakeup on all ports */
> - if (hcd->self.root_hub->do_remote_wakeup)
> - t2 |= PORT_WAKE_BITS;
> - else
> + if (hcd->self.root_hub->do_remote_wakeup) {
> + 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
Why is this change needed? Are your controllers non-compliant with the
spec, causing them to generate wakeup notifications at the wrong time?
The spec says, for example, that if PORT_WKCONN_E is set then a
suspended controller will generate a wakeup request when a connection
event occurs. But if the port was already connected then there is no
connection event.
> t2 &= ~PORT_WAKE_BITS;
>
> if (t1 != t2) {
> ehci_vdbg (ehci, "port %d, %08x -> %08x\n",
> port + 1, t1, t2);
> ehci_writel(ehci, t2, reg);
> + if (hostpc_reg) {
> + u32 t3;
> + msleep(5);
It's a bad idea to inclde this delay here unless your controller has
only one port.
> + t3 = ehci_readl(ehci, hostpc_reg);
> + ehci_writel(ehci, t3 | HOSTPC_PHCD, hostpc_reg);
> + t3 = ehci_readl(ehci, hostpc_reg);
> + ehci_dbg(ehci, "Port%d phy low pwr mode %s\n",
> + port, (t3 & HOSTPC_PHCD) ?
> + "succeeded" : "failed");
> + }
> }
> }
>
> @@ -242,7 +262,8 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
> (temp & PORT_SUSPEND)) {
> temp |= PORT_RESUME;
> resume_needed = 1;
> - }
> + } else if ((temp & (PORT_SUSPEND) && (temp & PORT_CONNECT)))
> + temp |= PORT_WKOC_E | PORT_WKDISC_E;
This code should not be needed. See below.
> ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
> }
>
> @@ -563,7 +584,8 @@ static int ehci_hub_control (
> int ports = HCS_N_PORTS (ehci->hcs_params);
> u32 __iomem *status_reg = &ehci->regs->port_status[
> (wIndex & 0xff) - 1];
> - u32 temp, status;
> + u32 __iomem *hostpc_reg = NULL;
> + u32 temp, temp1, status;
> unsigned long flags;
> int retval = 0;
> unsigned selector;
> @@ -576,6 +598,9 @@ static int ehci_hub_control (
> */
>
> spin_lock_irqsave (&ehci->lock, flags);
> + if (ehci->has_hostpc)
> + hostpc_reg = (u32 __iomem *)((unsigned char *)ehci->regs
> + + HOSTPC0 + 4 * ((wIndex & 0xff) - 1));
This statement should come before the spin_lock_irqsave(), not after.
> switch (typeReq) {
> case ClearHubFeature:
> switch (wValue) {
> @@ -773,7 +798,11 @@ static int ehci_hub_control (
> if (temp & PORT_CONNECT) {
> status |= 1 << USB_PORT_FEAT_CONNECTION;
> // status may be from integrated TT
> - status |= ehci_port_speed(ehci, temp);
> + if (ehci->has_hostpc) {
> + temp1 = ehci_readl(ehci, hostpc_reg);
> + status |= ehci_port_speed(ehci, temp1);
> + } else
> + status |= ehci_port_speed(ehci, temp);
> }
> if (temp & PORT_PE)
> status |= 1 << USB_PORT_FEAT_ENABLE;
> @@ -831,7 +860,25 @@ static int ehci_hub_control (
> if ((temp & PORT_PE) == 0
> || (temp & PORT_RESET) != 0)
> goto error;
> + if (temp & PORT_CONNECT) {
Unnecessary test -- since the port is enabled, it must be connected.
> + temp &= ~PORT_WKCONN_E;
> + temp |= (PORT_WKDISC_E | PORT_WKOC_E);
> + } else {
> + temp |= (PORT_WKCONN_E | PORT_WKOC_E);
> + temp &= ~PORT_WKDISC_E;
> + }
This isn't needed either. According the EHCI spec, disconnect and
overcurrent events on a suspended port will cause the Port Change
Detect bit to be set and an interrupt to be generated, even if
PORT_WKDISC_E and PORT_WKOC_E are clear.
> ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
> + /* Put phy in low power suspend with HOSTPCx reg */
> + if (hostpc_reg) {
> + msleep(5);
> + temp1 = ehci_readl(ehci, hostpc_reg);
> + ehci_writel(ehci, temp1 | HOSTPC_PHCD,
> + hostpc_reg);
> + temp1 = ehci_readl(ehci, hostpc_reg);
> + ehci_dbg(ehci, "Port%d phy low pwr mode %s\n",
> + wIndex, (temp1 & HOSTPC_PHCD) ?
> + "succeeded" : "failed");
> + }
> set_bit(wIndex, &ehci->suspended_ports);
> break;
> case USB_PORT_FEAT_POWER:
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index d04d018..3cfc3cc 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -135,6 +135,7 @@ struct ehci_hcd { /* one per controller */
> #define OHCI_HCCTRL_OFFSET 0x4
> #define OHCI_HCCTRL_LEN 0x4
> __hc32 *ohci_hcctrl_reg;
> + unsigned has_hostpc:1;
>
> u8 sbrn; /* packed release number */
>
> @@ -542,6 +543,14 @@ static inline unsigned int
> ehci_port_speed(struct ehci_hcd *ehci, unsigned int portsc)
> {
> if (ehci_is_TDI(ehci)) {
> + if (ehci->has_hostpc) {
> + if (portsc & (1<<25))
> + return 1<<USB_PORT_FEAT_LOWSPEED;
> + if (portsc & (1<<26))
> + return 1<<USB_PORT_FEAT_HIGHSPEED;
> + else
> + return 0; /* Full speed */
> + }
With all this extra code added in, ehci_port_speed() is now too big to
be inlined. Change it to a normal function.
Alan Stern
--
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
