diff mbox

[v2,08/10] usb: chipidea: host: add quirk for ehci operation

Message ID 1382423019-26184-9-git-send-email-peter.chen@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Chen Oct. 22, 2013, 6:23 a.m. UTC
For chipidea controller, it does not follow ehci spec strictly.
Taking resume signal as an example, it will stop resume signal about
20-21ms later automatically, but standard ehci spec says, the resume
signal is controlled by software (clear portsc.PORT_RESUME).

This operation causes some remote wakeup problems for high speed
devices due to host controller does not send SOF in time since
software can't guarantee set run/stop bit in time (run/stop bit
was cleared at the ehci suspend routine).

When software sets run/stop bit, it needs 1 SoF time to make it effect.
If we close the PHY clock just after setting run/stop bit, it does
not be set in practice, so a software delay is needed.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/host.c |   51 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

Comments

Alan Stern Oct. 23, 2013, 2:42 p.m. UTC | #1
On Tue, 22 Oct 2013, Peter Chen wrote:

> For chipidea controller, it does not follow ehci spec strictly.
> Taking resume signal as an example, it will stop resume signal about
> 20-21ms later automatically, but standard ehci spec says, the resume
> signal is controlled by software (clear portsc.PORT_RESUME).
> 
> This operation causes some remote wakeup problems for high speed
> devices due to host controller does not send SOF in time since
> software can't guarantee set run/stop bit in time (run/stop bit
> was cleared at the ehci suspend routine).
> 
> When software sets run/stop bit, it needs 1 SoF time to make it effect.
> If we close the PHY clock just after setting run/stop bit, it does
> not be set in practice, so a software delay is needed.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/host.c |   51 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 59e6020..283b385 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -33,6 +33,53 @@
>  #include "host.h"
>  
>  static struct hc_driver __read_mostly ci_ehci_hc_driver;
> +static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> +
> +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) {
> +			/*
> +			 * 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 path 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.
> +			 */
> +			if (hcd->self.root_hub->do_remote_wakeup) {
> +				ehci_dbg(ehci,
> +					"Remote wakeup is enabled, "
> +					"and device is on the port\n");
> +
> +				tmp = ehci_readl(ehci, &ehci->regs->command);
> +				tmp |= CMD_RUN;
> +				ehci_writel(ehci, tmp, &ehci->regs->command);
> +				/*
> +				 * It needs a short delay between set RUNSTOP
> +				 * and set PHCD.
> +				 */
> +				udelay(125);
> +			}
> +		}
> +	}

This code is a little strange.  If there is only one port then you 
don't need the loop.  But if there is more than one port then you don't 
want to put the udelay inside the loop.  (If two devices are attached, 
you don't want to delay the suspend by 250 us.)

Perhaps you should add a "break;" statement following the udelay.

Alan Stern
Peter Chen Oct. 24, 2013, 12:47 a.m. UTC | #2
On Wed, Oct 23, 2013 at 10:42:58AM -0400, Alan Stern wrote:
> On Tue, 22 Oct 2013, Peter Chen wrote:
> 
> > For chipidea controller, it does not follow ehci spec strictly.
> > Taking resume signal as an example, it will stop resume signal about
> > 20-21ms later automatically, but standard ehci spec says, the resume
> > signal is controlled by software (clear portsc.PORT_RESUME).
> > 
> > This operation causes some remote wakeup problems for high speed
> > devices due to host controller does not send SOF in time since
> > software can't guarantee set run/stop bit in time (run/stop bit
> > was cleared at the ehci suspend routine).
> > 
> > When software sets run/stop bit, it needs 1 SoF time to make it effect.
> > If we close the PHY clock just after setting run/stop bit, it does
> > not be set in practice, so a software delay is needed.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/chipidea/host.c |   51 +++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 51 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index 59e6020..283b385 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -33,6 +33,53 @@
> >  #include "host.h"
> >  
> >  static struct hc_driver __read_mostly ci_ehci_hc_driver;
> > +static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> > +
> > +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) {
> > +			/*
> > +			 * 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 path 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.
> > +			 */
> > +			if (hcd->self.root_hub->do_remote_wakeup) {
> > +				ehci_dbg(ehci,
> > +					"Remote wakeup is enabled, "
> > +					"and device is on the port\n");
> > +
> > +				tmp = ehci_readl(ehci, &ehci->regs->command);
> > +				tmp |= CMD_RUN;
> > +				ehci_writel(ehci, tmp, &ehci->regs->command);
> > +				/*
> > +				 * It needs a short delay between set RUNSTOP
> > +				 * and set PHCD.
> > +				 */
> > +				udelay(125);
> > +			}
> > +		}
> > +	}
> 
> This code is a little strange.  If there is only one port then you 
> don't need the loop.  But if there is more than one port then you don't 
> want to put the udelay inside the loop.  (If two devices are attached, 
> you don't want to delay the suspend by 250 us.)
> 

Yes, you are right. I will add break, that is once there is port connected,
delay 125us, then quit while.
Peter Chen Oct. 24, 2013, 5:50 a.m. UTC | #3
On Thu, Oct 24, 2013 at 07:51:03AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Peter Chen wrote:
> > For chipidea controller, it does not follow ehci spec strictly.
> > Taking resume signal as an example, it will stop resume signal about
> > 20-21ms later automatically, but standard ehci spec says, the resume
> > signal is controlled by software (clear portsc.PORT_RESUME).
> > 
> > This operation causes some remote wakeup problems for high speed
> > devices due to host controller does not send SOF in time since
> > software can't guarantee set run/stop bit in time (run/stop bit
> > was cleared at the ehci suspend routine).
> > 
> > When software sets run/stop bit, it needs 1 SoF time to make it effect.
> > If we close the PHY clock just after setting run/stop bit, it does
> > not be set in practice, so a software delay is needed.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/chipidea/host.c |   51 +++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 51 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index 59e6020..283b385 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -33,6 +33,53 @@
> >  #include "host.h"
> >  
> >  static struct hc_driver __read_mostly ci_ehci_hc_driver;
> > +static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> > +
> > +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) {
> > +			/*
> > +			 * 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 path 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.
> > +			 */
> > +			if (hcd->self.root_hub->do_remote_wakeup) {
> > +				ehci_dbg(ehci,
> > +					"Remote wakeup is enabled, "
> > +					"and device is on the port\n");
> >
> Please don't line wrap message texts, since it makes it harder to grep
> the kernel source for messages found in a logfile.
> 
> 

Thanks, will change.
Lothar Waßmann Oct. 24, 2013, 5:51 a.m. UTC | #4
Hi,

Peter Chen wrote:
> For chipidea controller, it does not follow ehci spec strictly.
> Taking resume signal as an example, it will stop resume signal about
> 20-21ms later automatically, but standard ehci spec says, the resume
> signal is controlled by software (clear portsc.PORT_RESUME).
> 
> This operation causes some remote wakeup problems for high speed
> devices due to host controller does not send SOF in time since
> software can't guarantee set run/stop bit in time (run/stop bit
> was cleared at the ehci suspend routine).
> 
> When software sets run/stop bit, it needs 1 SoF time to make it effect.
> If we close the PHY clock just after setting run/stop bit, it does
> not be set in practice, so a software delay is needed.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/host.c |   51 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 59e6020..283b385 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -33,6 +33,53 @@
>  #include "host.h"
>  
>  static struct hc_driver __read_mostly ci_ehci_hc_driver;
> +static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> +
> +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) {
> +			/*
> +			 * 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 path 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.
> +			 */
> +			if (hcd->self.root_hub->do_remote_wakeup) {
> +				ehci_dbg(ehci,
> +					"Remote wakeup is enabled, "
> +					"and device is on the port\n");
>
Please don't line wrap message texts, since it makes it harder to grep
the kernel source for messages found in a logfile.


Lothar Waßmann
diff mbox

Patch

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 59e6020..283b385 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -33,6 +33,53 @@ 
 #include "host.h"
 
 static struct hc_driver __read_mostly ci_ehci_hc_driver;
+static int (*orig_bus_suspend)(struct usb_hcd *hcd);
+
+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) {
+			/*
+			 * 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 path 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.
+			 */
+			if (hcd->self.root_hub->do_remote_wakeup) {
+				ehci_dbg(ehci,
+					"Remote wakeup is enabled, "
+					"and device is on the port\n");
+
+				tmp = ehci_readl(ehci, &ehci->regs->command);
+				tmp |= CMD_RUN;
+				ehci_writel(ehci, tmp, &ehci->regs->command);
+				/*
+				 * It needs a short delay between set RUNSTOP
+				 * and set PHCD.
+				 */
+				udelay(125);
+			}
+		}
+	}
+
+	return 0;
+}
 
 static irqreturn_t host_irq(struct ci_hdrc *ci)
 {
@@ -134,5 +181,9 @@  int ci_hdrc_host_init(struct ci_hdrc *ci)
 
 	ehci_init_driver(&ci_ehci_hc_driver, NULL);
 
+	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
+
+	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
+
 	return 0;
 }