2012/3/15 Xu, Andiry <Andiry.Xu@xxxxxxx>: >> -----Original Message----- >> From: Elric Fu [mailto:elricfu1@xxxxxxxxx] >> Sent: Thursday, March 15, 2012 3:41 PM >> To: Xu, Andiry >> Cc: Sarah Sharp; linux-usb@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 2/2] xHCI: fix bug of resume for VIA xHCI host >> >> 2012/3/15 Xu, Andiry <Andiry.Xu@xxxxxxx>: >> >> -----Original Message----- >> >> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- >> >> owner@xxxxxxxxxxxxxxx] On Behalf Of Elric Fu >> >> Sent: Thursday, March 15, 2012 1:46 PM >> >> To: Sarah Sharp >> >> Cc: Elric Fu; linux-usb@xxxxxxxxxxxxxxx >> >> Subject: [PATCH 2/2] xHCI: fix bug of resume for VIA xHCI host >> >> >> >> The Save/Restore state funtion of VIA xHCI host is unstable. So add >> >> the XHCI_RESET_ON_RESUME quirk for it. >> >> >> > >> > Can you provide more background of the issue? What's the unstable >> > behavior of VIA host after resume? >> >> The issue of using the Save/Restore state operation will cause the >> device attached to the port of host is disconnected. >> Sorry. My description about the issue is incorrect. If using the save/restore state function, after resume, the superspeed devices work fine but the usb 2.0 devices will be disconnected. It seems like the embedded 2.0 hub in VIA host controller can't be re-enumerated, so the 2.0 devices will be disconnected. I don't know why. > > Hmm. I think xHCI suspend path may need to be save DCBAA and all related > data structures before remove core well power. It's specified by xHCI > 1.0 errata. Well. Saving the DCBAA, Context and other data structures maybe fix this kind of issue. > >> > >> >> When driver free Event Ring in the resume procedure, driver can't >> >> write zero to Event Ring Registers in VIA xHCI host, otherwise the >> >> VIA host can't be resumed. >> >> >> > >> > Strange. What do you mean by "can't resume"? Does xhci_resume() return >> > failure, or it returns success but host does not work after resume? >> >> The issue is the host will hang up. The reason is that the host has a >> hardware state machine, in the reset-on-resume procedure driver will >> reset the host first then free event ring and re-initialize the host. But >> if driver set 0 to event ring registers in xhci_mem_cleanup() after reset >> the host, the host will think the "0" is a vaild value, and fetch it. it >> result >> in the host hang up. >> > > xhci_halt() is called before xhci_reset(). Why does the host fetch event ring > registers after it's halted? I don't know. As far as I know, after reset the VIA host, the host will fetch the value of event ring register as long as the driver write those registers. Then after driver write 0 to those registers the host thinks the value is valid, maybe do something to event ring after fetching the value. I don't know the more details about it. > >> > >> > If the host needs a reset-on-resume, it will perform a complete >> > re-initialization. After xhci_mem_cleanup() called, xhci_resume() will >> > call xhci_init(), in which the ERST registers are re-written. Have you >> > checked the value, see if it's different form before suspend? >> >> I don't think in reset-on-resume write 0 to event Ring registers is >> necessary. >> As driver already reset the host before xhci_mem_cleanup(), the value of >> event ring registers will be 0. >> >> > >> > Also, note xhci_mem_cleanup is called not only by xhci_resume(), but also >> > by xhci_stop(). This means you may find the same issue by rmmod/modprobe >> > xhci-hcd. >> >> rmmod/modprobe xhci-hcd is ok. I have already tested it. I think it is >> ok if we write >> a valid value except for 0 to event ring registers in VIA xHCI host >> after reset the >> host. >> > > But xhci_stop() also calls xhci_reset(), then xhci_mem_cleanup(). It also > writes 0 to ERST registers after reset the host. Hmm, I see. I think after the xhci-hcd driver is insmoded again, driver will reset the host again in host initialization. It can recover the host. Best Regards, Elric Fu > > Thanks, > Andiry > > >> > >> >> Signed-off-by: Elric Fu <elricfu1@xxxxxxxxx> >> >> --- >> >> drivers/usb/host/xhci-mem.c | 2 +- >> >> drivers/usb/host/xhci-pci.c | 1 + >> >> 2 files changed, 2 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >> >> index 383fc85..9321837 100644 >> >> --- a/drivers/usb/host/xhci-mem.c >> >> +++ b/drivers/usb/host/xhci-mem.c >> >> @@ -1690,7 +1690,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) >> >> int i; >> >> >> >> /* Free the Event Ring Segment Table and the actual Event Ring */ >> >> - if (xhci->ir_set) { >> >> + if (xhci->ir_set && !(xhci->quirks & XHCI_VIA_HOST)) { >> >> xhci_writel(xhci, 0, &xhci->ir_set->erst_size); >> >> xhci_write_64(xhci, 0, &xhci->ir_set->erst_base); >> >> xhci_write_64(xhci, 0, &xhci->ir_set->erst_dequeue); >> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >> >> index 872d024..75350ea 100644 >> >> --- a/drivers/usb/host/xhci-pci.c >> >> +++ b/drivers/usb/host/xhci-pci.c >> >> @@ -106,6 +106,7 @@ static void xhci_pci_quirks(struct device *dev, >> struct >> >> xhci_hcd *xhci) >> >> } >> >> if (pdev->vendor == PCI_VENDOR_ID_VIA) { >> >> xhci->quirks |= XHCI_VIA_HOST; >> >> + xhci->quirks |= XHCI_RESET_ON_RESUME; >> >> xhci_via_get_fw(xhci, pdev); >> >> } >> >> } >> >> -- >> >> 1.7.1 >> >> >> >> -- >> >> 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 >> > >> > > > -- 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
