Re: Test needed: ehci(4) suspend/resume rework

2013-06-03 Thread Martin Pieuchot 
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, act);
-   ehci_shutdown(sc);
+   ehci_reset(sc);

Re: Test needed: ehci(4) suspend/resume rework

2013-05-02 Thread Martin Pieuchot 
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 bios sucks */
-   EOWRITE4(sc, EHCI_CTRLDSSEGMENT, 0);
+

Re: Test needed: ehci(4) suspend/resume rework

2013-05-02 Thread patrick keshishian 
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-miscm=136327530312197w=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: Generic-, Multi-Card, 
1.00 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: Generic-, Multi-Card, 
1.00 SCSI0 0/direct removable serial.0bda015811417340
May  2 10:53:33 noir

Re: Test needed: ehci(4) suspend/resume rework

2013-05-01 Thread Ted Unangst 
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

2013-04-30 Thread Martin Pieuchot 
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-miscm=136327530312197w=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

2013-04-29 Thread patrick keshishian 
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-miscm=136327530312197w=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_ASS | EHCI_STS_PSS);
   if (hcr == 0)
   break;
 -
 - usb_delay_ms(sc-sc_bus,

Test needed: ehci(4) suspend/resume rework

2013-04-28 Thread Martin Pieuchot 
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;
-   EOWRITE4(sc, EHCI_USBCMD, cmd);
-
-