On Sat, May 19, 2012 at 12:43 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 18 May 2012, Alan Stern wrote: > >> On Fri, 18 May 2012, Chen Peter-B29397 wrote: >> >> > Hi Ming, >> > >> > I have debugged it at this afternoon, your description for this problem >> > is correct, the ehci_reset override the ehci->command which is assigned >> > at ehci_init, and ehci->command at ehci_init is correct. >> > >> > My opinion for this fix is just remove the >> > " ehci->command = ehci_readl(ehci, &ehci->regs->command);" at ehci_reset. >> > Even someone called ehci_reset to reset controller, it can still use command >> > value assigned at ehci_init. >> >> Yes, I see the problem. And you are right; the value assigned during >> ehci_init() should not be overridden. >> >> However... The value assigned in ehci_init() is not fully correct. >> That routine does not examine the default value in the USBCMD register >> before making its own settings. (It can't -- the USBCMD register may >> contain random garbage because the controller hasn't been reset at this >> point.) >> >> This means that ehci_init() can't handle hardware updates. New >> versions of the hardware may define bits in USBCMD that the code >> doesn't know about. Those bits would be set properly by default when a >> hardware reset occurs, but then ehci_init() would wipe them out. >> >> It looks like a bunch of code needs to be moved from ehci_init() to >> ehci_reset() -- everything involved in computing ehci->command. The >> idea is that once the controller has been reset, we first set >> ehci->command to the value in the USBCMD register (as is done now). >> Then we can modify the bits we know about (as the code in ehci_init() >> does) while leaving the other bits alone. > > A simpler idea just occurred to me. All we need to do is change the > line in ehci_reset() that assigns ehci->command. > > What it should do is mask out the bits that were set up in ehci_init(). > Those bits should be kept unchanged in ehci->command. The remaining > bits should be set according to the value read from the USBCMD > register. IMO, it is not a good idea. Firstly, after writing RESET on command register, the default value of CMD register should be restored to '00080000h (00080B00h if Asynchronous Schedule Park Capability is a one)'[1], and both 'Interrupt Threshold Control' and 'Park mode' has been set in ehci_init(), so using ehci->command after ehci_reset is correct. Secondly, remaining other bits from HW is not good since HW may be buggy and the default value after reset may be uncertain, and it is better to not depend on them. For some newer hardware, if the reserved fields are to be used, just do it in ehci_init like setting lpm bits. Finally, before the commit 3d9545cc375d117554a9b35dfddadf9189c62775( EHCI: maintain the ehci->command value properly), the ehci->command is used just directly in ehci_run and the other HW bits are not kept from HW, so it is better to be consistent as before to avoid introducing possible regression. [1], 4.1 Host Controller Initialization of EHCI Specification 1.0 > Would anyone like to code that up? Looks removing the line of 'ehci->command = ehci_readl(ehci, &ehci->regs->command);' in ehci_reset is OK. Thanks, -- Ming Lei -- 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
