> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Alan Stern > Sent: Tuesday, April 17, 2012 12:25 PM Hi Alan, > The ehci-hcd driver is a little haphazard about keeping track of the > state of the USBCMD register. The ehci->command field is supposed to > hold the register's value (apart from a few special bits) at all > times, but it isn't maintained properly. > > This patch (as1543) cleans up the situation. It keeps ehci->command > up-to-date, and uses that value rather than reading the register from > the hardware whenever possible. > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > --- > > drivers/usb/host/ehci-dbg.c | 6 ++---- > drivers/usb/host/ehci-hcd.c | 18 +++++++++++------- > drivers/usb/host/ehci-hub.c | 2 +- > drivers/usb/host/ehci-q.c | 16 ++++++---------- > drivers/usb/host/ehci-sched.c | 10 ++++------ > 6 files changed, 29 insertions(+), 37 deletions(-) > > Index: usb-3.4/drivers/usb/host/ehci-dbg.c > =================================================================== > --- usb-3.4.orig/drivers/usb/host/ehci-dbg.c > +++ usb-3.4/drivers/usb/host/ehci-dbg.c > @@ -1032,10 +1032,8 @@ static ssize_t debug_lpm_write(struct fi > if (strict_strtoul(buf + 5, 16, &hird)) > return -EINVAL; > printk(KERN_INFO "setting hird %s %lu\n", buf + 6, hird); > - temp = ehci_readl(ehci, &ehci->regs->command); > - temp &= ~CMD_HIRD; > - temp |= hird << 24; > - ehci_writel(ehci, temp, &ehci->regs->command); > + ehci->command = (ehci->command & ~CMD_HIRD) | (hird << 24); > + ehci_writel(ehci, ehci->command, &ehci->regs->command); > } else if (strncmp(buf, "disable", 7) == 0) { > if (strict_strtoul(buf + 8, 10, &port)) > return -EINVAL; > Index: usb-3.4/drivers/usb/host/ehci-hcd.c > =================================================================== > --- usb-3.4.orig/drivers/usb/host/ehci-hcd.c > +++ usb-3.4/drivers/usb/host/ehci-hcd.c > @@ -226,8 +226,13 @@ static int ehci_halt (struct ehci_hcd *e > if ((temp & STS_HALT) != 0) > return 0; > > + /* > + * This routine gets called during probe before ehci->command > + * has been initialized, so we can't rely on its value. > + */ > + ehci->command &= ~CMD_RUN; > temp = ehci_readl(ehci, &ehci->regs->command); > - temp &= ~CMD_RUN; > + temp &= ~(CMD_RUN | CMD_IAAD); > ehci_writel(ehci, temp, &ehci->regs->command); > return handshake (ehci, &ehci->regs->status, > STS_HALT, STS_HALT, 16 * 125); > @@ -347,6 +352,7 @@ static int ehci_reset (struct ehci_hcd * > if (ehci->debug) > dbgp_external_startup(); > > + ehci->command = ehci_readl(ehci, &ehci->regs->command); Wouldn't it be a good idea to mask off bits that cannot be cleared by software (like CMD_IAAD) whenever ehci->command is read from the register? Otherwise, if a bit has been set but not cleared by the hardware yet, that bit would get set again the next time you use the contents of ehci->command. > ehci->port_c_suspend = ehci->suspended_ports = > ehci->resuming_ports = 0; > return retval; > @@ -363,16 +369,14 @@ static void ehci_quiesce (struct ehci_hc > #endif > > /* wait for any schedule enables/disables to take effect */ > - temp = ehci_readl(ehci, &ehci->regs->command) << 10; > - temp &= STS_ASS | STS_PSS; > + temp = (ehci->command << 10) & (STS_ASS | STS_PSS); > if (handshake_on_error_set_halt(ehci, &ehci->regs->status, > STS_ASS | STS_PSS, temp, 16 * 125)) > return; > > /* then disable anything that's still active */ > - temp = ehci_readl(ehci, &ehci->regs->command); > - temp &= ~(CMD_ASE | CMD_IAAD | CMD_PSE); > - ehci_writel(ehci, temp, &ehci->regs->command); > + ehci->command &= ~(CMD_ASE | CMD_PSE); > + ehci_writel(ehci, ehci->command, &ehci->regs->command); > > /* hardware can take 16 microframes to turn off ... */ > handshake_on_error_set_halt(ehci, &ehci->regs->status, > Index: usb-3.4/drivers/usb/host/ehci-hub.c > =================================================================== > --- usb-3.4.orig/drivers/usb/host/ehci-hub.c > +++ usb-3.4/drivers/usb/host/ehci-hub.c > @@ -233,7 +233,6 @@ static int ehci_bus_suspend (struct usb_ > /* stop schedules, clean any completed work */ > if (ehci->rh_state == EHCI_RH_RUNNING) > ehci_quiesce (ehci); > - ehci->command = ehci_readl(ehci, &ehci->regs->command); > ehci_work(ehci); > > /* Unlike other USB host controller types, EHCI doesn't have > @@ -374,6 +373,7 @@ static int ehci_bus_resume (struct usb_h > ehci_writel(ehci, (u32) ehci->async->qh_dma, &ehci->regs->async_next); > > /* restore CMD_RUN, framelist size, and irq threshold */ > + ehci->command |= CMD_RUN; > ehci_writel(ehci, ehci->command, &ehci->regs->command); > ehci->rh_state = EHCI_RH_RUNNING; > > Index: usb-3.4/drivers/usb/host/ehci-q.c > =================================================================== > --- usb-3.4.orig/drivers/usb/host/ehci-q.c > +++ usb-3.4/drivers/usb/host/ehci-q.c > @@ -981,14 +981,12 @@ static void qh_link_async (struct ehci_h > head = ehci->async; > timer_action_done (ehci, TIMER_ASYNC_OFF); > if (!head->qh_next.qh) { > - u32 cmd = ehci_readl(ehci, &ehci->regs->command); > - > - if (!(cmd & CMD_ASE)) { > + if (!(ehci->command & CMD_ASE)) { > /* in case a clear of CMD_ASE didn't take yet */ > (void)handshake(ehci, &ehci->regs->status, > STS_ASS, 0, 150); > - cmd |= CMD_ASE; > - ehci_writel(ehci, cmd, &ehci->regs->command); > + ehci->command |= CMD_ASE; > + ehci_writel(ehci, ehci->command, &ehci->regs->command); It seems that the comment is not valid anymore. Since you are not reading from the command register here, there is no way to tell whether "a clear of CMD_ASE didn't take yet". It seems this is a change in the behavior of the driver. -- Paul -- 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
