On Mon, Dec 29, 2014 at 10:44:28AM -0500, Alan Stern wrote:
> On Mon, 29 Dec 2014, Peter Chen wrote:
>
> > For chipidea, its resume sequence is not-EHCI compatible, see
> > below description for FPR at portsc. So in order to send SoF in
> > time for remote wakeup sequence(within 3ms), the RUN/STOP bit must
> > be set before the resume signal is ended.
> >
> > Force Port Resume - RW. Default = 0b.
> > 1= Resume detected/driven on port.
> > 0=No resume (K-state) detected/driven on port.
> > Host mode:
> > Software sets this bit to one to drive resume signaling. The Controller sets this bit to '1' if
> > a J-to-K transition is detected while the port is in the Suspend state. When this bit
> > transitions to a '1' because a J-to-K transition is detected, the Port Change Detect bit in
> > the USBSTS register is also set to '1'. This bit will automatically change to '0' after the
> > resume sequence is complete. This behavior is different from EHCI where the controller
> > driver is required to set this bit to a '0' after the resume duration is timed in the driver.
> > Note that when the controller owns the port, the resume sequence follows the defined
> >
> > sequence documented in the USB Specification Revision 2.0. The resume signaling
> > (Full-speed 'K') is driven on the port as long as this bit remains a '1'. This bit will remain
> > a '1' until the port has switched to idle. Writing a '0' has no affect because the port
> > controller will time the resume operation, clear the bit and the port control state switches
> > to HS or FS idle.
> > This field is '0' if Port Power(PP) is '0' in host mode.
> >
> > This bit is not-EHCI compatible.
>
> If you really want to do this, go ahead. But see below.
>
Any other solutions for this (Let the SoF send in time within 3ms after resume
signal has ended)?
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> > ---
> > drivers/usb/chipidea/host.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index 48731d0..4f2fd3b 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -33,6 +33,7 @@
> > #include "host.h"
> >
> > static struct hc_driver __read_mostly ci_ehci_hc_driver;
> > +static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> >
> > struct ehci_ci_priv {
> > struct regulator *reg_vbus;
> > @@ -158,6 +159,50 @@ void ci_hdrc_host_destroy(struct ci_hdrc *ci)
> > host_stop(ci);
> > }
> >
> > +static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
> > +{
> > + struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > + int port;
> > + u32 tmp;
> > +
> > + int ret = orig_bus_suspend(hcd);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + port = HCS_N_PORTS(ehci->hcs_params);
> > + while (port--) {
> > + u32 __iomem *reg = &ehci->regs->port_status[port];
> > + u32 portsc = ehci_readl(ehci, reg);
> > +
> > + if ((portsc & PORT_CONNECT) &&
> > + hcd->self.root_hub->do_remote_wakeup) {
>
> This is not the right test. You want to know whether remote wakeup is
> enabled for any downstream devices, not whether it is enabled for the
> root hub.
>
> It would be simpler to get rid of this test, and always turn on the
> CMD_RUN bit.
>
What I had thought was:
- There is device on the root hub.
- If the user wants to enable downstream device's remote wakeup, he must
enable root hub's as well
Yes, it is not the right test, I will use simpler, and only add
connection judgement.
> > + /*
> > + * For chipidea, the resume signal will be ended
> > + * automatically, so for remote wakeup case, the
> > + * usbcmd.rs may not be set before the resume has
> > + * ended if other resume paths consumes too much
> > + * time (~23ms-24ms), in that case, the SOF will not
> > + * send out within 3ms after resume ends, then the
> > + * device will enter suspend again.
> > + */
> > + ehci_dbg(ehci, "Remote wakeup is enabled for device\n");
> > +
> > + tmp = ehci_readl(ehci, &ehci->regs->command);
> > + tmp |= CMD_RUN;
> > + ehci_writel(ehci, tmp, &ehci->regs->command);
>
> At this point, the bus isn't suspended any more. It is running.
>
The bus state is 'J' since the portsc.suspendM = 1, so the bus is
suspended, correct?
> > + /*
> > + * It needs a short delay between set RUNSTOP
> > + * and set PHCD.
> > + */
> > + udelay(125);
>
> Wouldn't it be better to do a sleep instead of a delay?
>
Sure, will change to usleep_range.
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int ci_hdrc_host_init(struct ci_hdrc *ci)
> > {
> > struct ci_role_driver *rdrv;
> > @@ -176,6 +221,8 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
> > ci->roles[CI_ROLE_HOST] = rdrv;
> >
> > ehci_init_driver(&ci_ehci_hc_driver, &ehci_ci_overrides);
> > + orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
> > + ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
> >
> > return 0;
> > }
>
> Alan Stern
>
--
Best Regards,
Peter Chen
--
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
