On 01/16/2013 08:08 AM, Alan Stern wrote: > On Wed, 16 Jan 2013, Venu Byravarasu wrote: > >> As Tegra PHY driver needs to access one of the Host registers, >> added few APIs. >> --- a/drivers/usb/host/ehci-tegra.c >> +++ b/drivers/usb/host/ehci-tegra.c >> +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable) >> +{ >> + unsigned long val; >> + struct usb_hcd *hcd = bus_to_hcd(x->otg->host); >> + void __iomem *base = hcd->regs; >> + u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN; >> + >> + val = readl(base + USB_PORTSC1); >> + if (enable) >> + val |= wake; >> + else >> + val &= ~wake; >> + writel(val, base + USB_PORTSC1); >> +} > > Here and below, this sort of code is highly questionable. You > evidently don't realize that some of the bits in the PORTSC registers > are R/WC. This means writing a 1 to these bits will clear them. > > Consequently it is almost always wrong to read a PORTSC register and > then write back the same (or a slightly modified) value. Sorry I'm not familiar with USB... Are the bits being manipulated here (i.e. USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN) standardized USBisms, or some custom Tegra stuff? Anyway, is the solution here to do: val = readl(addr) // i.e. add the following line: val &= ~(all write-to-clear bits); if (enable) val |= wake; else val &= ~wake; writel(val, addr) ... or is there more broken than just that? Also note that the driver is already doing exactly what is in these new functions; the code is just being split out so that only the EHCI driver touches EHCI registers, and the PHY driver only touches PHY registers. Still, I'll admit it's a good time to fix any mistakes in this part of the code. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html