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.c        19 Apr 2013 08:58:53 -0000      1.131
> +++ sys/dev/usb/ehci.c        28 Apr 2013 12:32:43 -0000
> @@ -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);
> -
> -             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);
> +#if 0
> +             ehci_reset(sc);
> +#endif
> +
> +             cparams = EREAD4(sc, EHCI_HCCPARAMS);
> +             /* MUST clear segment register if 64 bit capable. */
> +             if (EHCI_HCC_64BIT(cparams))
> +                     EWRITE4(sc, EHCI_CTRLDSSEGMENT, 0);
> +
>               EOWRITE4(sc, EHCI_PERIODICLISTBASE, DMAADDR(&sc->sc_fldma, 0));
>               EOWRITE4(sc, EHCI_ASYNCLISTADDR,
>                   sc->sc_async_head->physaddr | EHCI_LINK_QH);
> -             EOWRITE4(sc, EHCI_USBINTR, sc->sc_eintrs);
>  
> -             hcr = 0;
> -             for (i = 1; i <= sc->sc_noport; i++) {
> -                     cmd = EOREAD4(sc, EHCI_PORTSC(i));
> -                     if ((cmd & (EHCI_PS_PO|EHCI_PS_SUSP)) == EHCI_PS_SUSP) {
> -                             EOWRITE4(sc, EHCI_PORTSC(i),
> -                                 cmd | EHCI_PS_FPR);
> -                             hcr = 1;
> -                     }
> -             }
> -
> -             if (hcr) {
> -                     usb_delay_ms(&sc->sc_bus, USB_RESUME_WAIT);
> -                     for (i = 1; i <= sc->sc_noport; i++) {
> -                             cmd = EOREAD4(sc, EHCI_PORTSC(i));
> -                             if ((cmd & (EHCI_PS_PO|EHCI_PS_SUSP)) ==
> -                                 EHCI_PS_SUSP)
> -                                     EOWRITE4(sc, EHCI_PORTSC(i),
> -                                         cmd & ~EHCI_PS_FPR);
> -                     }
> -             }
> -
> -             EOWRITE4(sc, EHCI_USBCMD, sc->sc_cmd);
> +             /* Turn on controller */
> +             EOWRITE4(sc, EHCI_USBCMD,
> +                 EHCI_CMD_ITC_2 | /* 2 microframes interrupt delay */
> +                 (EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_FLS_M) |
> +                 EHCI_CMD_ASE |
> +                 EHCI_CMD_PSE |
> +                 EHCI_CMD_RS);
>  
>               /* Take over port ownership */
>               EOWRITE4(sc, EHCI_CONFIGFLAG, EHCI_CONF_CF);
> -
>               for (i = 0; i < 100; i++) {
> +                     usb_delay_ms(&sc->sc_bus, 1);
>                       hcr = EOREAD4(sc, EHCI_USBSTS) & EHCI_STS_HCH;
> -                     if (hcr != EHCI_STS_HCH)
> +                     if (!hcr)
>                               break;
> +             }
>  
> -                     usb_delay_ms(&sc->sc_bus, 1);
> +             if (hcr) {
> +                     printf("%s: run timeout\n", sc->sc_bus.bdev.dv_xname);
> +                     /* XXX should we bail here? */
>               }
> -             if (hcr == EHCI_STS_HCH)
> -                     printf("%s: config timeout\n",
> -                         sc->sc_bus.bdev.dv_xname);
> +
> +             EOWRITE4(sc, EHCI_USBINTR, sc->sc_eintrs);
>  
>               usb_delay_ms(&sc->sc_bus, USB_RESUME_WAIT);
>  
> @@ -1157,17 +1128,38 @@ ehci_activate(struct device *self, int a
>       return (rv);
>  }
>  
> -/*
> - * Shut down the controller when the system is going down.
> - */
> -void
> -ehci_shutdown(void *v)
> +usbd_status
> +ehci_reset(struct ehci_softc *sc)
>  {
> -     struct ehci_softc *sc = v;
> +     u_int32_t hcr;
> +     int i;
>  
> -     DPRINTF(("ehci_shutdown: stopping the HC\n"));
> +     DPRINTF(("%s: stopping the HC\n", __func__));
>       EOWRITE4(sc, EHCI_USBCMD, 0);   /* Halt controller */
> +     for (i = 0; i < 100; i++) {
> +             usb_delay_ms(&sc->sc_bus, 1);
> +             hcr = EOREAD4(sc, EHCI_USBSTS) & EHCI_STS_HCH;
> +             if (hcr)
> +                     break;
> +     }
> +
> +     if (!hcr)
> +             printf("%s: halt timeout\n", sc->sc_bus.bdev.dv_xname);
> +
>       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);
> +     }
> +
> +     return (USBD_NORMAL_COMPLETION);
>  }
>  
>  struct usbd_xfer *
> Index: sys/dev/usb/ehcivar.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ehcivar.h,v
> retrieving revision 1.25
> diff -u -p -r1.25 ehcivar.h
> --- sys/dev/usb/ehcivar.h     15 Apr 2013 09:23:01 -0000      1.25
> +++ sys/dev/usb/ehcivar.h     27 Apr 2013 09:53:29 -0000
> @@ -125,8 +125,6 @@ struct ehci_softc {
>       char sc_vendor[16];             /* vendor string for root hub */
>       int sc_id_vendor;               /* vendor ID for root hub */
>  
> -     u_int32_t sc_cmd;               /* shadow of cmd reg during suspend */
> -
>       struct usb_dma sc_fldma;
>       ehci_link_t *sc_flist;
>       u_int sc_flsize;
> @@ -180,4 +178,5 @@ usbd_status       ehci_init(struct ehci_softc 
>  int          ehci_intr(void *);
>  int          ehci_detach(struct ehci_softc *, int);
>  int          ehci_activate(struct device *, int);
> -void         ehci_shutdown(void *);
> +usbd_status  ehci_reset(struct ehci_softc *);
> +
> Index: sys/arch/beagle/dev/omehci.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/beagle/dev/omehci.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 omehci.c
> --- sys/arch/beagle/dev/omehci.c      20 Apr 2013 17:43:53 -0000      1.12
> +++ sys/arch/beagle/dev/omehci.c      27 Apr 2013 09:37:09 -0000
> @@ -179,7 +179,7 @@ omehci_activate(struct device *self, int
>               sc->sc.sc_bus.use_polling--;
>               break;
>       case DVACT_POWERDOWN:
> -             ehci_shutdown(sc);
> +             ehci_reset(sc->sc);
>               break;
>       }
>       return 0;
> 

Reply via email to