Re: Test needed: ehci(4) suspend/resume rework
> > Ok so, here's an updated diff with the reset enable in the resume case. > > Updated diff after recent ehci(4) changes. > > I'm still looking for non-regression tests. Awesome to see ehci suspend/resume getting some love; I will get to testing this in a few hours.
Re: Test needed: ehci(4) suspend/resume rework
On 02/05/13(Thu) 09:41, Martin Pieuchot wrote:
> On 01/05/13(Wed) 17:44, Ted Unangst wrote:
> > On Sun, Apr 28, 2013 at 15:44, Martin Pieuchot wrote:
> > > Diff below is a rework of the suspend/resume logic in ehci(4).
> > >
> >
> > > In case this diff doesn't help or if you have a problem when resuming,
> > > I left an "#ifdef 0" block in the DVACT_RESUME. Try enabling it and tell
> > > me if it changes something.
> >
> > Got around to testing this. Now everything works. It still prints
> > echi_idone about 100 times after resume, but it doesn't print it
> > forever.
> >
> > I'd say the diff works, but only with the reset in the resume case as
> > well.
>
> Ok so, here's an updated diff with the reset enable in the resume case.
Updated diff after recent ehci(4) changes.
I'm still looking for non-regression tests.
Index: dev/usb/ehci.c
===
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.133
diff -u -p -r1.133 ehci.c
--- dev/usb/ehci.c 30 May 2013 16:15:02 - 1.133
+++ dev/usb/ehci.c 3 Jun 2013 13:49:17 -
@@ -353,22 +353,10 @@ ehci_init(struct ehci_softc *sc)
sc->sc_bus.usbrev = USBREV_2_0;
- /* Reset the controller */
DPRINTF(("%s: resetting\n", sc->sc_bus.bdev.dv_xname));
- EOWRITE4(sc, EHCI_USBCMD, 0); /* Halt controller */
- usb_delay_ms(&sc->sc_bus, 1);
- EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
- for (i = 0; i < 100; i++) {
- usb_delay_ms(&sc->sc_bus, 1);
- hcr = EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_HCRESET;
- if (!hcr)
- break;
- }
- if (hcr) {
- printf("%s: reset timeout\n",
- sc->sc_bus.bdev.dv_xname);
- return (USBD_IOERROR);
- }
+ err = ehci_reset(sc);
+ if (err)
+ return (err);
/* frame list size at default, read back what we got and use that */
switch (EHCI_CMD_FLS(EOREAD4(sc, EHCI_USBCMD))) {
@@ -1033,6 +1021,8 @@ ehci_detach(struct ehci_softc *sc, int f
timeout_del(&sc->sc_tmo_intrlist);
+ ehci_reset(sc);
+
usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
/* XXX free other data structures XXX */
@@ -1045,7 +1035,7 @@ int
ehci_activate(struct device *self, int act)
{
struct ehci_softc *sc = (struct ehci_softc *)self;
- u_int32_t cmd, hcr;
+ u_int32_t cmd, hcr, cparams;
int i, rv = 0;
switch (act) {
@@ -1056,95 +1046,74 @@ ehci_activate(struct device *self, int a
rv = config_activate_children(self, act);
sc->sc_bus.use_polling++;
- for (i = 1; i <= sc->sc_noport; i++) {
- cmd = EOREAD4(sc, EHCI_PORTSC(i));
- if ((cmd & (EHCI_PS_PO|EHCI_PS_PE)) == EHCI_PS_PE)
- EOWRITE4(sc, EHCI_PORTSC(i),
- cmd | EHCI_PS_SUSP);
- }
-
- sc->sc_cmd = EOREAD4(sc, EHCI_USBCMD);
- cmd = sc->sc_cmd & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
+ /*
+* First tell the host to stop processing Asynchronous
+* and Periodic schedules.
+*/
+ cmd = EOREAD4(sc, EHCI_USBCMD) & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
EOWRITE4(sc, EHCI_USBCMD, cmd);
-
for (i = 0; i < 100; i++) {
+ usb_delay_ms(&sc->sc_bus, 1);
hcr = EOREAD4(sc, EHCI_USBSTS) &
(EHCI_STS_ASS | EHCI_STS_PSS);
if (hcr == 0)
break;
-
- usb_delay_ms(&sc->sc_bus, 1);
}
if (hcr != 0)
- printf("%s: reset timeout\n",
+ printf("%s: disable schedules timeout\n",
sc->sc_bus.bdev.dv_xname);
- cmd &= ~EHCI_CMD_RS;
- EOWRITE4(sc, EHCI_USBCMD, cmd);
-
- for (i = 0; i < 100; i++) {
- hcr = EOREAD4(sc, EHCI_USBSTS) & EHCI_STS_HCH;
- if (hcr == EHCI_STS_HCH)
- break;
-
- usb_delay_ms(&sc->sc_bus, 1);
- }
- if (hcr != EHCI_STS_HCH)
- printf("%s: config timeout\n",
- sc->sc_bus.bdev.dv_xname);
+ /*
+* Then reset the host as if it was a shutdown.
+*
+* All USB devices are disconnected/reconnected during
+* a suspend/resume cycle so keep it simple.
+*/
+ ehci_reset(sc);
sc->sc_bus.use_polling--;
break;
case DVACT_POWERDOWN:
rv = config_activate_children(self,
Re: Test needed: ehci(4) suspend/resume rework
On Tue, Apr 30, 2013 at 09:51:21AM +0200, Martin Pieuchot wrote: > On 29/04/13(Mon) 13:25, patrick keshishian wrote: > > On Sun, Apr 28, 2013 at 03:44:09PM +0200, Martin Pieuchot wrote: > > > Diff below is a rework of the suspend/resume logic in ehci(4). > > > > > > If you're not interested in the full story below, please make sure > > > it doesn't introduce any regression on your machine(s) and, in any > > > case report to me (with your dmesg!) thanks :) > > > > > > Full story: > > > > > > So this diff changes the way we supsend/resume ehci(4) in various > > > way. It started as a simple rewrite of the ehci_reset() function > > > to fix a race while working on the rework of the suspend/resume > > > framework with deraadt@, then later on I discovered that some of > > > the magic in there was wrong or no longer needed. > > > > > > First of all, it removes the logic introduced in r1.46 that places > > > all the ports into suspend mode because it no longer makes sense > > > as we *do* detach/reattach USB devices during a suspend/resume > > > cycle now. > > > > Naive question here, but would this be a cause for issue > > reported here: > > > > http://marc.info/?l=openbsd-misc&m=136327530312197&w=2 > > Certainly. > > > > > or at least I'm curious as to why the Wacom tablet does not > > reattach after a resume. > > Really? Are you sure it doesn't reattach or is it just not usable in > X? It's easy to check, try to suspend/resume in console with the > tablet connected, does it show up again in your dmesg? I'm pretty sure it did not, at least at the time when I posted the original message (ref above) to misc@. As noted in that post, the little "hey, i'm active" LED on the Wacom tablet would not turn on after resume. However, I had some trouble reproducing this condition since your reply. This behavior seems USB port dependent: $ tail -f /var/log/messages May 2 10:51:47 noir /bsd: uhidev0 at uhub2 May 2 10:51:47 noir /bsd: port 1 configuration 1 interface 0 "WACOM ET-0405-UV1.1-1" rev 1.00/1.11 addr 3 May 2 10:51:47 noir /bsd: uhidev0: iclass 3/1, 3 report ids May 2 10:51:47 noir /bsd: uhid0 at uhidev0 reportid 2: input=7, output=0, feature=2 May 2 10:51:47 noir /bsd: uhid1 at uhidev0 reportid 3: input=0, output=0, feature=2 May 2 10:52:31 noir apmd: system resumed from APM sleep May 2 10:52:31 noir /bsd: sd1 detached May 2 10:52:32 noir /bsd: scsibus1 detached May 2 10:52:32 noir /bsd: umass0 detached May 2 10:52:32 noir /bsd: umass0 at uhub1 May 2 10:52:32 noir /bsd: port 2 configuration 1 interface 0 "Generic USB2.0-CRW" rev 2.00/58.88 addr 2 May 2 10:52:32 noir /bsd: umass0: using SCSI over Bulk-Only May 2 10:52:32 noir /bsd: scsibus1 at umass0: 2 targets, initiator 0 May 2 10:52:32 noir /bsd: sd1 at scsibus1 targ 1 lun 0: SCSI0 0/direct removable serial.0bda015811417340 May 2 10:52:32 noir /bsd: video0 detached May 2 10:52:32 noir /bsd: uvideo0 detached May 2 10:52:33 noir /bsd: uvideo0 at uhub1 May 2 10:52:34 noir /bsd: port 5 configuration 1 interface 0 "Image Processor Integrated Camera" rev 2.00/0.29 addr 3 May 2 10:52:34 noir /bsd: video0 at uvideo0 May 2 10:52:34 noir /bsd: uhid0 detached May 2 10:52:34 noir /bsd: uhid1 detached May 2 10:52:34 noir /bsd: uhidev0 detached May 2 10:52:34 noir /bsd: uhidev0 at uhub2 May 2 10:52:34 noir /bsd: port 1 configuration 1 interface 0 "WACOM ET-0405-UV1.1-1" rev 1.00/1.11 addr 3 May 2 10:52:34 noir /bsd: uhidev0: iclass 3/1, 3 report ids May 2 10:52:34 noir /bsd: uhid0 at uhidev0 reportid 2: input=7, output=0, feature=2 May 2 10:52:34 noir /bsd: uhid1 at uhidev0 reportid 3: input=0, output=0, feature=2 May 2 10:52:34 noir /bsd: ugen0 detached May 2 10:52:35 noir /bsd: ugen0 at uhub2 May 2 10:52:35 noir /bsd: port 4 "Broadcom Corp Broadcom Bluetooth Device" rev 2.00/7.48 addr 2 Switching USB ports the Wacom is connected to: May 2 10:52:46 noir /bsd: uhid0 detached May 2 10:52:46 noir /bsd: uhid1 detached May 2 10:52:46 noir /bsd: uhidev0 detached May 2 10:52:50 noir /bsd: uhidev0 at uhub2 May 2 10:52:50 noir /bsd: port 3 configuration 1 interface 0 "WACOM ET-0405-UV1.1-1" rev 1.00/1.11 addr 3 May 2 10:52:50 noir /bsd: uhidev0: iclass 3/1, 3 report ids May 2 10:52:50 noir /bsd: uhid0 at uhidev0 reportid 2: input=7, output=0, feature=2 May 2 10:52:50 noir /bsd: uhid1 at uhidev0 reportid 3: input=0, output=0, feature=2 May 2 10:53:32 noir apmd: system resumed from APM sleep May 2 10:53:32 noir /bsd: sd1 detached May 2 10:53:33 noir /bsd: scsibus1 detached May 2 10:53:33 noir /bsd: umass0 detached May 2 10:53:33 noir /bsd: umass0 at uhub1 May 2 10:53:33 noir /bsd: port 2 configuration 1 interface 0 "Generic USB2.0-CRW" rev 2.00/58.88 addr 2 May 2 10:53:33 noir /bsd: umass0: using SCSI over Bulk-Only May 2 10:53:33 noir /bsd: scsibus1 at umass0: 2 targets, initiator 0 May 2 10:53:33 noir /bsd: sd1 at scsibus1 targ 1 lun 0: SCSI0 0/direct removable serial.0bd
Re: Test needed: ehci(4) suspend/resume rework
On 01/05/13(Wed) 17:44, Ted Unangst wrote:
> On Sun, Apr 28, 2013 at 15:44, Martin Pieuchot wrote:
> > Diff below is a rework of the suspend/resume logic in ehci(4).
> >
>
> > In case this diff doesn't help or if you have a problem when resuming,
> > I left an "#ifdef 0" block in the DVACT_RESUME. Try enabling it and tell
> > me if it changes something.
>
> Got around to testing this. Now everything works. It still prints
> echi_idone about 100 times after resume, but it doesn't print it
> forever.
>
> I'd say the diff works, but only with the reset in the resume case as
> well.
Ok so, here's an updated diff with the reset enable in the resume case.
Please test and report to me.
Martin
Index: sys/dev/usb/ehci.c
===
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.131
diff -u -p -r1.131 ehci.c
--- sys/dev/usb/ehci.c 19 Apr 2013 08:58:53 - 1.131
+++ sys/dev/usb/ehci.c 2 May 2013 07:37:01 -
@@ -353,22 +353,10 @@ ehci_init(struct ehci_softc *sc)
sc->sc_bus.usbrev = USBREV_2_0;
- /* Reset the controller */
DPRINTF(("%s: resetting\n", sc->sc_bus.bdev.dv_xname));
- EOWRITE4(sc, EHCI_USBCMD, 0); /* Halt controller */
- usb_delay_ms(&sc->sc_bus, 1);
- EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
- for (i = 0; i < 100; i++) {
- usb_delay_ms(&sc->sc_bus, 1);
- hcr = EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_HCRESET;
- if (!hcr)
- break;
- }
- if (hcr) {
- printf("%s: reset timeout\n",
- sc->sc_bus.bdev.dv_xname);
- return (USBD_IOERROR);
- }
+ err = ehci_reset(sc);
+ if (err)
+ return (err);
/* frame list size at default, read back what we got and use that */
switch (EHCI_CMD_FLS(EOREAD4(sc, EHCI_USBCMD))) {
@@ -1032,6 +1020,8 @@ ehci_detach(struct ehci_softc *sc, int f
timeout_del(&sc->sc_tmo_intrlist);
+ ehci_reset(sc);
+
usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
/* XXX free other data structures XXX */
@@ -1044,7 +1034,7 @@ int
ehci_activate(struct device *self, int act)
{
struct ehci_softc *sc = (struct ehci_softc *)self;
- u_int32_t cmd, hcr;
+ u_int32_t cmd, hcr, cparams;
int i, rv = 0;
switch (act) {
@@ -1054,94 +1044,73 @@ ehci_activate(struct device *self, int a
case DVACT_SUSPEND:
sc->sc_bus.use_polling++;
- for (i = 1; i <= sc->sc_noport; i++) {
- cmd = EOREAD4(sc, EHCI_PORTSC(i));
- if ((cmd & (EHCI_PS_PO|EHCI_PS_PE)) == EHCI_PS_PE)
- EOWRITE4(sc, EHCI_PORTSC(i),
- cmd | EHCI_PS_SUSP);
- }
-
- sc->sc_cmd = EOREAD4(sc, EHCI_USBCMD);
- cmd = sc->sc_cmd & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
+ /*
+* First tell the host to stop processing Asynchronous
+* and Periodic schedules.
+*/
+ cmd = EOREAD4(sc, EHCI_USBCMD) & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
EOWRITE4(sc, EHCI_USBCMD, cmd);
-
for (i = 0; i < 100; i++) {
+ usb_delay_ms(&sc->sc_bus, 1);
hcr = EOREAD4(sc, EHCI_USBSTS) &
(EHCI_STS_ASS | EHCI_STS_PSS);
if (hcr == 0)
break;
-
- usb_delay_ms(&sc->sc_bus, 1);
}
if (hcr != 0)
- printf("%s: reset timeout\n",
+ printf("%s: disable schedules timeout\n",
sc->sc_bus.bdev.dv_xname);
- cmd &= ~EHCI_CMD_RS;
- EOWRITE4(sc, EHCI_USBCMD, cmd);
-
- for (i = 0; i < 100; i++) {
- hcr = EOREAD4(sc, EHCI_USBSTS) & EHCI_STS_HCH;
- if (hcr == EHCI_STS_HCH)
- break;
-
- usb_delay_ms(&sc->sc_bus, 1);
- }
- if (hcr != EHCI_STS_HCH)
- printf("%s: config timeout\n",
- sc->sc_bus.bdev.dv_xname);
+ /*
+* Then reset the host as if it was a shutdown.
+*
+* All USB devices are disconnected/reconnected during
+* a suspend/resume cycle so keep it simple.
+*/
+ ehci_reset(sc);
sc->sc_bus.use_polling--;
break;
case DVACT_POWERDOWN:
- ehci_shutdown(sc);
+ ehci_reset(sc);
break;
case DVACT_RESUME:
sc->sc_bus.use_polling++;
- /* restore things in case the bi
Re: Test needed: ehci(4) suspend/resume rework
On Sun, Apr 28, 2013 at 15:44, Martin Pieuchot wrote: > Diff below is a rework of the suspend/resume logic in ehci(4). > > In case this diff doesn't help or if you have a problem when resuming, > I left an "#ifdef 0" block in the DVACT_RESUME. Try enabling it and tell > me if it changes something. Got around to testing this. Now everything works. It still prints echi_idone about 100 times after resume, but it doesn't print it forever. I'd say the diff works, but only with the reset in the resume case as well.
Re: Test needed: ehci(4) suspend/resume rework
On 29/04/13(Mon) 13:25, patrick keshishian wrote: > On Sun, Apr 28, 2013 at 03:44:09PM +0200, Martin Pieuchot wrote: > > Diff below is a rework of the suspend/resume logic in ehci(4). > > > > If you're not interested in the full story below, please make sure > > it doesn't introduce any regression on your machine(s) and, in any > > case report to me (with your dmesg!) thanks :) > > > > Full story: > > > > So this diff changes the way we supsend/resume ehci(4) in various > > way. It started as a simple rewrite of the ehci_reset() function > > to fix a race while working on the rework of the suspend/resume > > framework with deraadt@, then later on I discovered that some of > > the magic in there was wrong or no longer needed. > > > > First of all, it removes the logic introduced in r1.46 that places > > all the ports into suspend mode because it no longer makes sense > > as we *do* detach/reattach USB devices during a suspend/resume > > cycle now. > > Naive question here, but would this be a cause for issue > reported here: > > http://marc.info/?l=openbsd-misc&m=136327530312197&w=2 Certainly. > > or at least I'm curious as to why the Wacom tablet does not > reattach after a resume. Really? Are you sure it doesn't reattach or is it just not usable in X? It's easy to check, try to suspend/resume in console with the tablet connected, does it show up again in your dmesg? And for the crash you can try to add the following in your xorg.conf: Section "ServerFlags" Option "AllowMouseOpenFail" "True" EndSection I don't know if it will be enough, I'm not sure what's the status of devices hotplugging in xenocara. If you want to be more than "curious" you can build the xf86-input-usbtablet with debug enable and try to find what the problem is ;) Diffs are always welcome! M.
Re: Test needed: ehci(4) suspend/resume rework
On Sun, Apr 28, 2013 at 03:44:09PM +0200, Martin Pieuchot wrote:
> Diff below is a rework of the suspend/resume logic in ehci(4).
>
> If you're not interested in the full story below, please make sure
> it doesn't introduce any regression on your machine(s) and, in any
> case report to me (with your dmesg!) thanks :)
>
> Full story:
>
> So this diff changes the way we supsend/resume ehci(4) in various
> way. It started as a simple rewrite of the ehci_reset() function
> to fix a race while working on the rework of the suspend/resume
> framework with deraadt@, then later on I discovered that some of
> the magic in there was wrong or no longer needed.
>
> First of all, it removes the logic introduced in r1.46 that places
> all the ports into suspend mode because it no longer makes sense
> as we *do* detach/reattach USB devices during a suspend/resume
> cycle now.
Naive question here, but would this be a cause for issue
reported here:
http://marc.info/?l=openbsd-misc&m=136327530312197&w=2
or at least I'm curious as to why the Wacom tablet does not
reattach after a resume.
--patrick
> Then it does a proper reset of the host while suspending instead
> of halting it and keeping the value of USBCMD. Now that we are
> simply shutting down any USB device during suspend I see no real
> advantage in keeping a complex suspend/resume logic. Just stop
> the host as if it was a shutdown and set it up again when resuming.
>
> To be coherent, it also modifies the resume path to match what's
> done in ehci_init(), fixing some small differences (and potential
> bugs!). I could have merged the two functions, but I don't want to
> have a too big diff for now.
>
> I'd really like to know if this introduces any regression. I'm not
> sure it will fix any of the ehci(4) suspend problems I'm aware of
> 'cause I don't have such hardware, but I hope it will help :)
>
> In case this diff doesn't help or if you have a problem when resuming,
> I left an "#ifdef 0" block in the DVACT_RESUME. Try enabling it and tell
> me if it changes something.
>
> M.
>
>
> Index: sys/dev/usb/ehci.c
> ===
> RCS file: /cvs/src/sys/dev/usb/ehci.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 ehci.c
> --- sys/dev/usb/ehci.c19 Apr 2013 08:58:53 - 1.131
> +++ sys/dev/usb/ehci.c28 Apr 2013 12:32:43 -
> @@ -353,22 +353,10 @@ ehci_init(struct ehci_softc *sc)
>
> sc->sc_bus.usbrev = USBREV_2_0;
>
> - /* Reset the controller */
> DPRINTF(("%s: resetting\n", sc->sc_bus.bdev.dv_xname));
> - EOWRITE4(sc, EHCI_USBCMD, 0); /* Halt controller */
> - usb_delay_ms(&sc->sc_bus, 1);
> - EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
> - for (i = 0; i < 100; i++) {
> - usb_delay_ms(&sc->sc_bus, 1);
> - hcr = EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_HCRESET;
> - if (!hcr)
> - break;
> - }
> - if (hcr) {
> - printf("%s: reset timeout\n",
> - sc->sc_bus.bdev.dv_xname);
> - return (USBD_IOERROR);
> - }
> + err = ehci_reset(sc);
> + if (err)
> + return (err);
>
> /* frame list size at default, read back what we got and use that */
> switch (EHCI_CMD_FLS(EOREAD4(sc, EHCI_USBCMD))) {
> @@ -1032,6 +1020,8 @@ ehci_detach(struct ehci_softc *sc, int f
>
> timeout_del(&sc->sc_tmo_intrlist);
>
> + ehci_reset(sc);
> +
> usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
>
> /* XXX free other data structures XXX */
> @@ -1044,7 +1034,7 @@ int
> ehci_activate(struct device *self, int act)
> {
> struct ehci_softc *sc = (struct ehci_softc *)self;
> - u_int32_t cmd, hcr;
> + u_int32_t cmd, hcr, cparams;
> int i, rv = 0;
>
> switch (act) {
> @@ -1054,94 +1044,75 @@ ehci_activate(struct device *self, int a
> case DVACT_SUSPEND:
> sc->sc_bus.use_polling++;
>
> - for (i = 1; i <= sc->sc_noport; i++) {
> - cmd = EOREAD4(sc, EHCI_PORTSC(i));
> - if ((cmd & (EHCI_PS_PO|EHCI_PS_PE)) == EHCI_PS_PE)
> - EOWRITE4(sc, EHCI_PORTSC(i),
> - cmd | EHCI_PS_SUSP);
> - }
> -
> - sc->sc_cmd = EOREAD4(sc, EHCI_USBCMD);
> - cmd = sc->sc_cmd & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
> + /*
> + * First tell the host to stop processing Asynchronous
> + * and Periodic schedules.
> + */
> + cmd = EOREAD4(sc, EHCI_USBCMD) & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
> EOWRITE4(sc, EHCI_USBCMD, cmd);
> -
> for (i = 0; i < 100; i++) {
> + usb_delay_ms(&sc->sc_bus, 1);
> hcr = EOREAD4(sc, EHCI_USBSTS) &
> (EHCI_STS_A
Test needed: ehci(4) suspend/resume rework
Diff below is a rework of the suspend/resume logic in ehci(4).
If you're not interested in the full story below, please make sure
it doesn't introduce any regression on your machine(s) and, in any
case report to me (with your dmesg!) thanks :)
Full story:
So this diff changes the way we supsend/resume ehci(4) in various
way. It started as a simple rewrite of the ehci_reset() function
to fix a race while working on the rework of the suspend/resume
framework with deraadt@, then later on I discovered that some of
the magic in there was wrong or no longer needed.
First of all, it removes the logic introduced in r1.46 that places
all the ports into suspend mode because it no longer makes sense
as we *do* detach/reattach USB devices during a suspend/resume
cycle now.
Then it does a proper reset of the host while suspending instead
of halting it and keeping the value of USBCMD. Now that we are
simply shutting down any USB device during suspend I see no real
advantage in keeping a complex suspend/resume logic. Just stop
the host as if it was a shutdown and set it up again when resuming.
To be coherent, it also modifies the resume path to match what's
done in ehci_init(), fixing some small differences (and potential
bugs!). I could have merged the two functions, but I don't want to
have a too big diff for now.
I'd really like to know if this introduces any regression. I'm not
sure it will fix any of the ehci(4) suspend problems I'm aware of
'cause I don't have such hardware, but I hope it will help :)
In case this diff doesn't help or if you have a problem when resuming,
I left an "#ifdef 0" block in the DVACT_RESUME. Try enabling it and tell
me if it changes something.
M.
Index: sys/dev/usb/ehci.c
===
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.131
diff -u -p -r1.131 ehci.c
--- sys/dev/usb/ehci.c 19 Apr 2013 08:58:53 - 1.131
+++ sys/dev/usb/ehci.c 28 Apr 2013 12:32:43 -
@@ -353,22 +353,10 @@ ehci_init(struct ehci_softc *sc)
sc->sc_bus.usbrev = USBREV_2_0;
- /* Reset the controller */
DPRINTF(("%s: resetting\n", sc->sc_bus.bdev.dv_xname));
- EOWRITE4(sc, EHCI_USBCMD, 0); /* Halt controller */
- usb_delay_ms(&sc->sc_bus, 1);
- EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
- for (i = 0; i < 100; i++) {
- usb_delay_ms(&sc->sc_bus, 1);
- hcr = EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_HCRESET;
- if (!hcr)
- break;
- }
- if (hcr) {
- printf("%s: reset timeout\n",
- sc->sc_bus.bdev.dv_xname);
- return (USBD_IOERROR);
- }
+ err = ehci_reset(sc);
+ if (err)
+ return (err);
/* frame list size at default, read back what we got and use that */
switch (EHCI_CMD_FLS(EOREAD4(sc, EHCI_USBCMD))) {
@@ -1032,6 +1020,8 @@ ehci_detach(struct ehci_softc *sc, int f
timeout_del(&sc->sc_tmo_intrlist);
+ ehci_reset(sc);
+
usb_delay_ms(&sc->sc_bus, 300); /* XXX let stray task complete */
/* XXX free other data structures XXX */
@@ -1044,7 +1034,7 @@ int
ehci_activate(struct device *self, int act)
{
struct ehci_softc *sc = (struct ehci_softc *)self;
- u_int32_t cmd, hcr;
+ u_int32_t cmd, hcr, cparams;
int i, rv = 0;
switch (act) {
@@ -1054,94 +1044,75 @@ ehci_activate(struct device *self, int a
case DVACT_SUSPEND:
sc->sc_bus.use_polling++;
- for (i = 1; i <= sc->sc_noport; i++) {
- cmd = EOREAD4(sc, EHCI_PORTSC(i));
- if ((cmd & (EHCI_PS_PO|EHCI_PS_PE)) == EHCI_PS_PE)
- EOWRITE4(sc, EHCI_PORTSC(i),
- cmd | EHCI_PS_SUSP);
- }
-
- sc->sc_cmd = EOREAD4(sc, EHCI_USBCMD);
- cmd = sc->sc_cmd & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
+ /*
+* First tell the host to stop processing Asynchronous
+* and Periodic schedules.
+*/
+ cmd = EOREAD4(sc, EHCI_USBCMD) & ~(EHCI_CMD_ASE | EHCI_CMD_PSE);
EOWRITE4(sc, EHCI_USBCMD, cmd);
-
for (i = 0; i < 100; i++) {
+ usb_delay_ms(&sc->sc_bus, 1);
hcr = EOREAD4(sc, EHCI_USBSTS) &
(EHCI_STS_ASS | EHCI_STS_PSS);
if (hcr == 0)
break;
-
- usb_delay_ms(&sc->sc_bus, 1);
}
if (hcr != 0)
- printf("%s: reset timeout\n",
+ printf("%s: disable schedules timeout\n",
sc->sc_bus.bdev.dv_xname);
- cmd &= ~EHCI_CMD_RS;
- EOWR
