| Message ID | 1382423019-26184-10-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 22 Oct 2013, Peter Chen wrote: > When the port goes to suspend or finishes resme, it needs to > notify PHY, it is not a standard EHCI operation, so we add a > quirk for it. Actually, this _should_ be a standard EHCI operation. But we have to figure out a way to do it that will work on all platforms. Felipe, any ideas? Alan Stern > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/chipidea/host.c | 129 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb/chipidea.h | 1 + > 2 files changed, 130 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > index 283b385..c1d05c4 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -34,6 +34,10 @@ > > static struct hc_driver __read_mostly ci_ehci_hc_driver; > static int (*orig_bus_suspend)(struct usb_hcd *hcd); > +static int (*orig_bus_resume)(struct usb_hcd *hcd); > +static int (*orig_hub_control)(struct usb_hcd *hcd, > + u16 typeReq, u16 wValue, u16 wIndex, > + char *buf, u16 wLength); > > static int ci_ehci_bus_suspend(struct usb_hcd *hcd) > { > @@ -75,12 +79,131 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd) > */ > udelay(125); > } > + if (hcd->phy && test_bit(port, &ehci->bus_suspended) > + && (ehci_port_speed(ehci, portsc) == > + USB_PORT_STAT_HIGH_SPEED)) > + /* > + * notify the USB PHY, it is for global > + * suspend case. > + */ > + usb_phy_notify_suspend(hcd->phy, > + USB_SPEED_HIGH); > } > } > > return 0; > } > > +static int ci_imx_ehci_bus_resume(struct usb_hcd *hcd) > +{ > + struct ehci_hcd *ehci = hcd_to_ehci(hcd); > + int port; > + > + int ret = orig_bus_resume(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); > + /* > + * Notify PHY after resume signal has finished, it is > + * for global suspend case. > + */ > + if (hcd->phy > + && test_bit(port, &ehci->bus_suspended) > + && (portsc & PORT_CONNECT) > + && (ehci_port_speed(ehci, portsc) == > + USB_PORT_STAT_HIGH_SPEED)) > + /* notify the USB PHY */ > + usb_phy_notify_resume(hcd->phy, USB_SPEED_HIGH); > + } > + > + return 0; > +}
Hi, On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote: > On Tue, 22 Oct 2013, Peter Chen wrote: > > > When the port goes to suspend or finishes resme, it needs to > > notify PHY, it is not a standard EHCI operation, so we add a > > quirk for it. > > Actually, this _should_ be a standard EHCI operation. But we have to > figure out a way to do it that will work on all platforms. > > Felipe, any ideas? isn't it enough to call usb_phy_set_suspend() at apropriate places ? This should work on all platforms if the PHY driver is implemented correctly.
On Wed, 23 Oct 2013, Felipe Balbi wrote: > Hi, > > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote: > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > > > When the port goes to suspend or finishes resme, it needs to > > > notify PHY, it is not a standard EHCI operation, so we add a > > > quirk for it. > > > > Actually, this _should_ be a standard EHCI operation. But we have to > > figure out a way to do it that will work on all platforms. > > > > Felipe, any ideas? > > isn't it enough to call usb_phy_set_suspend() at apropriate places ? > > This should work on all platforms if the PHY driver is implemented > correctly. On Tue, 22 Oct 2013, Peter Chen wrote: > + /* > + * notify the USB PHY, it is for global > + * suspend case. > + */ > + usb_phy_notify_suspend(hcd->phy, > + USB_SPEED_HIGH); Hmmm. This calls usb_phy_notify_suspend(), and later on, usb_phy_notify_resume(). AFAICT, those routines don't exist. Instead we have usb_phy_set_suspend(), as Felipe mentioned. It has a different interface, because the second argument specifies whether we are entering or leaving suspend, not the connection speed. Peter, you need to straighten this out. Also, there's the question of where are the appropriate places to make the calls? After the root hub goes into suspend, I suppose. But when is the right time during resume? And what if there is more than one port on the root hub? Alan Stern
On Wed, Oct 23, 2013 at 02:45:39PM -0400, Alan Stern wrote: > On Wed, 23 Oct 2013, Felipe Balbi wrote: > > > Hi, > > > > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote: > > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > > > > > When the port goes to suspend or finishes resme, it needs to > > > > notify PHY, it is not a standard EHCI operation, so we add a > > > > quirk for it. > > > > > > Actually, this _should_ be a standard EHCI operation. But we have to > > > figure out a way to do it that will work on all platforms. > > > > > > Felipe, any ideas? > > > > isn't it enough to call usb_phy_set_suspend() at apropriate places ? > > > > This should work on all platforms if the PHY driver is implemented > > correctly. > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > + /* > > + * notify the USB PHY, it is for global > > + * suspend case. > > + */ > > + usb_phy_notify_suspend(hcd->phy, > > + USB_SPEED_HIGH); > > Hmmm. This calls usb_phy_notify_suspend(), and later on, > usb_phy_notify_resume(). AFAICT, those routines don't exist. > Hi Alan and Felipe, It is at my another patchset: "Add power management support for MXS PHY" http://marc.info/?l=linux-usb&m=138242248913823&w=2 Since they are two things, one is from PHY, the other is for controller. I split them to two patchset. For these two notifications, please see: [Patch v2 07/14] usb: phy: add notify suspend and resume callback http://marc.info/?l=linux-usb&m=138242252713848&w=2 [Patch v2 08/14] usb: phy-mxs: Add implementation of nofity_suspend and notify_resume http://marc.info/?l=linux-usb&m=138242253313856&w=2 > Instead we have usb_phy_set_suspend(), as Felipe mentioned. It has a > different interface, because the second argument specifies whether we > are entering or leaving suspend, not the connection speed. > > Peter, you need to straighten this out. .set_suspend is different with .notify_suspend. .set_suspend is called when the PHY enters suspend. .notify_suspend is called when port enters suspends (setting portsc.suspendM, and stop sending SOF). After calling .notify_suspend, the PHY may still need to access, eg, set PHY wakeup setting, etc. The .notify_suspend is in ehci operation, .set_suspend can be at controller driver, since when to put the PHY enters suspend may platform specific. > > Also, there's the question of where are the appropriate places to make > the calls? After the root hub goes into suspend, I suppose. But when > is the right time during resume? > If we can take it as standard EHCI operation, we need to put it at below places: For .notify_suspend, we need to put it after set portsc.suspendM. - For selective suspend, at ehci_hub_control - For global suspend, at ehci_bus_suspend For .notify_resume, we need to put it after clear portsc.fpr, it is similar for .notify_suspend - For selective suspend, at ehci_hub_control - For global suspend, at ehci_bus_resume
Hi, On Thu, Oct 24, 2013 at 09:33:26AM +0800, Peter Chen wrote: > On Wed, Oct 23, 2013 at 02:45:39PM -0400, Alan Stern wrote: > > On Wed, 23 Oct 2013, Felipe Balbi wrote: > > > > > Hi, > > > > > > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote: > > > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > > > > > > > When the port goes to suspend or finishes resme, it needs to > > > > > notify PHY, it is not a standard EHCI operation, so we add a > > > > > quirk for it. > > > > > > > > Actually, this _should_ be a standard EHCI operation. But we have to > > > > figure out a way to do it that will work on all platforms. > > > > > > > > Felipe, any ideas? > > > > > > isn't it enough to call usb_phy_set_suspend() at apropriate places ? > > > > > > This should work on all platforms if the PHY driver is implemented > > > correctly. > > > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > > > + /* > > > + * notify the USB PHY, it is for global > > > + * suspend case. > > > + */ > > > + usb_phy_notify_suspend(hcd->phy, > > > + USB_SPEED_HIGH); > > > > Hmmm. This calls usb_phy_notify_suspend(), and later on, > > usb_phy_notify_resume(). AFAICT, those routines don't exist. > > > > Hi Alan and Felipe, > > It is at my another patchset: "Add power management support for MXS PHY" > http://marc.info/?l=linux-usb&m=138242248913823&w=2 > Since they are two things, one is from PHY, the other is for controller. > I split them to two patchset. > > For these two notifications, please see: > [Patch v2 07/14] usb: phy: add notify suspend and resume callback > http://marc.info/?l=linux-usb&m=138242252713848&w=2 > > [Patch v2 08/14] usb: phy-mxs: Add implementation of > nofity_suspend and notify_resume > http://marc.info/?l=linux-usb&m=138242253313856&w=2 > > > Instead we have usb_phy_set_suspend(), as Felipe mentioned. It has a > > different interface, because the second argument specifies whether we > > are entering or leaving suspend, not the connection speed. > > > > Peter, you need to straighten this out. > > .set_suspend is different with .notify_suspend. > > .set_suspend is called when the PHY enters suspend. > .notify_suspend is called when port enters suspends (setting portsc.suspendM, > and stop sending SOF). > > After calling .notify_suspend, the PHY may still need to access, > eg, set PHY wakeup setting, etc. right, this just means we need usage counters for the PHY layer. So that usb_phy_set_suspend() only decreaments a counter and will only suspend the PHY when said counter reaches zero. Also, why can't you *always* setup wakeup events for the PHY ? Everytime you're about to suspend the PHY, enable its wakeup events. > The .notify_suspend is in ehci operation, .set_suspend can be at > controller driver, since when to put the PHY enters suspend may > platform specific. not really, what's platform specific is *HOW* to suspend the PHY. *WHERE* to tell it to suspend is really more of a policy decision. > > Also, there's the question of where are the appropriate places to make > > the calls? After the root hub goes into suspend, I suppose. But when > > is the right time during resume? > > > > If we can take it as standard EHCI operation, we need to put it at below > places: > > For .notify_suspend, we need to put it after set portsc.suspendM. why do you need this notification for suspendM ? PHY should automatically enter a lower power state automatically when suspendM signal is asserted.
On Thu, Oct 24, 2013 at 07:04:11AM -0500, Felipe Balbi wrote: > Hi, > > On Thu, Oct 24, 2013 at 09:33:26AM +0800, Peter Chen wrote: > > On Wed, Oct 23, 2013 at 02:45:39PM -0400, Alan Stern wrote: > > > On Wed, 23 Oct 2013, Felipe Balbi wrote: > > > > > > > Hi, > > > > > > > > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote: > > > > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > > > > > > > > > When the port goes to suspend or finishes resme, it needs to > > > > > > notify PHY, it is not a standard EHCI operation, so we add a > > > > > > quirk for it. > > > > > > > > > > Actually, this _should_ be a standard EHCI operation. But we have to > > > > > figure out a way to do it that will work on all platforms. > > > > > > > > > > Felipe, any ideas? > > > > > > > > isn't it enough to call usb_phy_set_suspend() at apropriate places ? > > > > > > > > This should work on all platforms if the PHY driver is implemented > > > > correctly. > > > > > > On Tue, 22 Oct 2013, Peter Chen wrote: > > > > > > > + /* > > > > + * notify the USB PHY, it is for global > > > > + * suspend case. > > > > + */ > > > > + usb_phy_notify_suspend(hcd->phy, > > > > + USB_SPEED_HIGH); > > > > > > Hmmm. This calls usb_phy_notify_suspend(), and later on, > > > usb_phy_notify_resume(). AFAICT, those routines don't exist. > > > > > > > Hi Alan and Felipe, > > > > It is at my another patchset: "Add power management support for MXS PHY" > > http://marc.info/?l=linux-usb&m=138242248913823&w=2 > > Since they are two things, one is from PHY, the other is for controller. > > I split them to two patchset. > > > > For these two notifications, please see: > > [Patch v2 07/14] usb: phy: add notify suspend and resume callback > > http://marc.info/?l=linux-usb&m=138242252713848&w=2 > > > > [Patch v2 08/14] usb: phy-mxs: Add implementation of > > nofity_suspend and notify_resume > > http://marc.info/?l=linux-usb&m=138242253313856&w=2 > > > > > Instead we have usb_phy_set_suspend(), as Felipe mentioned. It has a > > > different interface, because the second argument specifies whether we > > > are entering or leaving suspend, not the connection speed. > > > > > > Peter, you need to straighten this out. > > > > .set_suspend is different with .notify_suspend. > > > > .set_suspend is called when the PHY enters suspend. > > .notify_suspend is called when port enters suspends (setting portsc.suspendM, > > and stop sending SOF). > > > > After calling .notify_suspend, the PHY may still need to access, > > eg, set PHY wakeup setting, etc. > > right, this just means we need usage counters for the PHY layer. So that > usb_phy_set_suspend() only decreaments a counter and will only suspend > the PHY when said counter reaches zero. > > Also, why can't you *always* setup wakeup events for the PHY ? Everytime > you're about to suspend the PHY, enable its wakeup events. Most of cases, the user doesn't want USB wakeup event. At these cases, if the PHY's wakeup is set, when external signal changes (id/vbus/dpdm), the PHY will be woken up and more power consumption will be. > > > The .notify_suspend is in ehci operation, .set_suspend can be at > > controller driver, since when to put the PHY enters suspend may > > platform specific. > > not really, what's platform specific is *HOW* to suspend the PHY. > *WHERE* to tell it to suspend is really more of a policy decision. > How to suspend the PHY is indeed platform specific, but sometimes, the whole suspend PHY operation may not only include PHY register, but also needs controller register, Eg, for chipidea controller, if we have not used chipidea PHY, we need to control both controller register (portsc.phcd) and PHY's register (Different register mapping), so it is hard to include two operations at usb_phy_set_suspend, we have to put them at controller's suspend. > > > Also, there's the question of where are the appropriate places to make > > > the calls? After the root hub goes into suspend, I suppose. But when > > > is the right time during resume? > > > > > > > If we can take it as standard EHCI operation, we need to put it at below > > places: > > > > For .notify_suspend, we need to put it after set portsc.suspendM. > > why do you need this notification for suspendM ? PHY should > automatically enter a lower power state automatically when suspendM > signal is asserted. > For some PHYs, it may correct. For chipidea controller, it needs to set portsc.phcd to let the PHY enter suspend. The software needs to control it.
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 283b385..c1d05c4 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -34,6 +34,10 @@ static struct hc_driver __read_mostly ci_ehci_hc_driver; static int (*orig_bus_suspend)(struct usb_hcd *hcd); +static int (*orig_bus_resume)(struct usb_hcd *hcd); +static int (*orig_hub_control)(struct usb_hcd *hcd, + u16 typeReq, u16 wValue, u16 wIndex, + char *buf, u16 wLength); static int ci_ehci_bus_suspend(struct usb_hcd *hcd) { @@ -75,12 +79,131 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd) */ udelay(125); } + if (hcd->phy && test_bit(port, &ehci->bus_suspended) + && (ehci_port_speed(ehci, portsc) == + USB_PORT_STAT_HIGH_SPEED)) + /* + * notify the USB PHY, it is for global + * suspend case. + */ + usb_phy_notify_suspend(hcd->phy, + USB_SPEED_HIGH); } } return 0; } +static int ci_imx_ehci_bus_resume(struct usb_hcd *hcd) +{ + struct ehci_hcd *ehci = hcd_to_ehci(hcd); + int port; + + int ret = orig_bus_resume(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); + /* + * Notify PHY after resume signal has finished, it is + * for global suspend case. + */ + if (hcd->phy + && test_bit(port, &ehci->bus_suspended) + && (portsc & PORT_CONNECT) + && (ehci_port_speed(ehci, portsc) == + USB_PORT_STAT_HIGH_SPEED)) + /* notify the USB PHY */ + usb_phy_notify_resume(hcd->phy, USB_SPEED_HIGH); + } + + return 0; +} + +/* The below code is based on tegra ehci driver */ +static int ci_imx_ehci_hub_control( + struct usb_hcd *hcd, + u16 typeReq, + u16 wValue, + u16 wIndex, + char *buf, + u16 wLength +) +{ + struct ehci_hcd *ehci = hcd_to_ehci(hcd); + u32 __iomem *status_reg; + u32 temp; + unsigned long flags; + int retval = 0; + + status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1]; + + spin_lock_irqsave(&ehci->lock, flags); + + if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND) { + temp = ehci_readl(ehci, status_reg); + if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) { + retval = -EPIPE; + goto done; + } + + temp &= ~(PORT_RWC_BITS | PORT_WKCONN_E); + temp |= PORT_WKDISC_E | PORT_WKOC_E; + ehci_writel(ehci, temp | PORT_SUSPEND, status_reg); + + /* + * If a transaction is in progress, there may be a delay in + * suspending the port. Poll until the port is suspended. + */ + if (ehci_handshake(ehci, status_reg, PORT_SUSPEND, + PORT_SUSPEND, 5000)) + ehci_err(ehci, "timeout waiting for SUSPEND\n"); + + spin_unlock_irqrestore(&ehci->lock, flags); + if (ehci_port_speed(ehci, temp) == + USB_PORT_STAT_HIGH_SPEED && hcd->phy) { + /* notify the USB PHY */ + usb_phy_notify_suspend(hcd->phy, USB_SPEED_HIGH); + } + spin_lock_irqsave(&ehci->lock, flags); + + set_bit((wIndex & 0xff) - 1, &ehci->suspended_ports); + goto done; + } + + /* + * After resume has finished, it needs do some post resume + * operation for some SoCs. + */ + else if (typeReq == ClearPortFeature && + wValue == USB_PORT_FEAT_C_SUSPEND) { + + /* Make sure the resume has finished, it should be finished */ + if (ehci_handshake(ehci, status_reg, PORT_RESUME, 0, 25000)) + ehci_err(ehci, "timeout waiting for resume\n"); + + temp = ehci_readl(ehci, status_reg); + + if (ehci_port_speed(ehci, temp) == + USB_PORT_STAT_HIGH_SPEED && hcd->phy) { + /* notify the USB PHY */ + usb_phy_notify_resume(hcd->phy, USB_SPEED_HIGH); + } + } + + spin_unlock_irqrestore(&ehci->lock, flags); + + /* Handle the hub control events here */ + return orig_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength); +done: + spin_unlock_irqrestore(&ehci->lock, flags); + return retval; +} + static irqreturn_t host_irq(struct ci_hdrc *ci) { return usb_hcd_irq(ci->irq, ci->hcd); @@ -182,8 +305,14 @@ 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; + orig_bus_resume = ci_ehci_hc_driver.bus_resume; + orig_hub_control = ci_ehci_hc_driver.hub_control; ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend; + if (ci->platdata->flags & CI_HDRC_IMX_EHCI_QUIRK) { + ci_ehci_hc_driver.bus_resume = ci_imx_ehci_bus_resume; + ci_ehci_hc_driver.hub_control = ci_imx_ehci_hub_control; + } return 0; } diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index 667b46c..8ce3daf 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -25,6 +25,7 @@ struct ci_hdrc_platform_data { * but otg is not supported (no register otgsc). */ #define CI_HDRC_DUAL_ROLE_NOT_OTG BIT(4) +#define CI_HDRC_IMX_EHCI_QUIRK BIT(5) enum usb_dr_mode dr_mode; #define CI_HDRC_CONTROLLER_RESET_EVENT 0 #define CI_HDRC_CONTROLLER_STOPPED_EVENT 1
When the port goes to suspend or finishes resme, it needs to notify PHY, it is not a standard EHCI operation, so we add a quirk for it. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/host.c | 129 ++++++++++++++++++++++++++++++++++++++++++ include/linux/usb/chipidea.h | 1 + 2 files changed, 130 insertions(+), 0 deletions(-)