| Message ID | 1399434623-20383-5-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 6 May 2014, Maxime Ripard wrote: > From: Boris BREZILLON <boris.brezillon@free-electrons.com> > > On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to > be deasserted for the EHCI block to be usable. > > Add support for an optional reset controller that will be deasserted on > power off and asserted on power on. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Is this really a _reset_ line? That is, when you assert the reset line, does it actually reset the EHCI controller, or does it merely leave the controller in a partially powered-down state? The difference is important. During suspend, the controller is supposed to remember the state of the port connections as well as other settings. If it doesn't, the controller and all attached USB devices will have to be reinitialized every time the controller resumes, which will increase the latency. Alan Stern
On Wed, May 07, 2014 at 10:25:55AM -0400, Alan Stern wrote: > On Tue, 6 May 2014, Maxime Ripard wrote: > > > From: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > > On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to > > be deasserted for the EHCI block to be usable. > > > > Add support for an optional reset controller that will be deasserted on > > power off and asserted on power on. > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > Is this really a _reset_ line? That is, when you assert the reset > line, does it actually reset the EHCI controller, or does it merely > leave the controller in a partially powered-down state? It actually resets the whole controller. > The difference is important. During suspend, the controller is > supposed to remember the state of the port connections as well as other > settings. If it doesn't, the controller and all attached USB devices > will have to be reinitialized every time the controller resumes, which > will increase the latency. So you're saying that we should move this to the probe then? Thanks, Maxime
Hi, On 05/08/2014 12:00 AM, Maxime Ripard wrote: > On Wed, May 07, 2014 at 10:25:55AM -0400, Alan Stern wrote: >> On Tue, 6 May 2014, Maxime Ripard wrote: >> >>> From: Boris BREZILLON <boris.brezillon@free-electrons.com> >>> >>> On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to >>> be deasserted for the EHCI block to be usable. >>> >>> Add support for an optional reset controller that will be deasserted on >>> power off and asserted on power on. >>> >>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> >> Is this really a _reset_ line? That is, when you assert the reset >> line, does it actually reset the EHCI controller, or does it merely >> leave the controller in a partially powered-down state? > > It actually resets the whole controller. > >> The difference is important. During suspend, the controller is >> supposed to remember the state of the port connections as well as other >> settings. If it doesn't, the controller and all attached USB devices >> will have to be reinitialized every time the controller resumes, which >> will increase the latency. > > So you're saying that we should move this to the probe then? Yes. Regards, Hans
On Thu, 8 May 2014, Hans de Goede wrote: > Hi, > > On 05/08/2014 12:00 AM, Maxime Ripard wrote: > > On Wed, May 07, 2014 at 10:25:55AM -0400, Alan Stern wrote: > >> On Tue, 6 May 2014, Maxime Ripard wrote: > >> > >>> From: Boris BREZILLON <boris.brezillon@free-electrons.com> > >>> > >>> On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to > >>> be deasserted for the EHCI block to be usable. > >>> > >>> Add support for an optional reset controller that will be deasserted on > >>> power off and asserted on power on. > >>> > >>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >> > >> Is this really a _reset_ line? That is, when you assert the reset > >> line, does it actually reset the EHCI controller, or does it merely > >> leave the controller in a partially powered-down state? > > > > It actually resets the whole controller. > > > >> The difference is important. During suspend, the controller is > >> supposed to remember the state of the port connections as well as other > >> settings. If it doesn't, the controller and all attached USB devices > >> will have to be reinitialized every time the controller resumes, which > >> will increase the latency. > > > > So you're saying that we should move this to the probe then? > > Yes. That's right. The controller should not be reset during suspend, if you can possibly avoid it. There isn't any real benefit to asserting the reset signal during suspend, is there? I mean, it won't use any less power, right? Alan Stern
On Thu, May 08, 2014 at 10:07:25AM -0400, Alan Stern wrote: > On Thu, 8 May 2014, Hans de Goede wrote: > > > Hi, > > > > On 05/08/2014 12:00 AM, Maxime Ripard wrote: > > > On Wed, May 07, 2014 at 10:25:55AM -0400, Alan Stern wrote: > > >> On Tue, 6 May 2014, Maxime Ripard wrote: > > >> > > >>> From: Boris BREZILLON <boris.brezillon@free-electrons.com> > > >>> > > >>> On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to > > >>> be deasserted for the EHCI block to be usable. > > >>> > > >>> Add support for an optional reset controller that will be deasserted on > > >>> power off and asserted on power on. > > >>> > > >>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > >> > > >> Is this really a _reset_ line? That is, when you assert the reset > > >> line, does it actually reset the EHCI controller, or does it merely > > >> leave the controller in a partially powered-down state? > > > > > > It actually resets the whole controller. > > > > > >> The difference is important. During suspend, the controller is > > >> supposed to remember the state of the port connections as well as other > > >> settings. If it doesn't, the controller and all attached USB devices > > >> will have to be reinitialized every time the controller resumes, which > > >> will increase the latency. > > > > > > So you're saying that we should move this to the probe then? > > > > Yes. > > That's right. The controller should not be reset during suspend, if > you can possibly avoid it. > > There isn't any real benefit to asserting the reset signal during > suspend, is there? I mean, it won't use any less power, right? I don't think it will. I'll update the patches and send a new version. Thanks! Maxime
diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt index ff151ec084c4..43c1a4e06767 100644 --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt @@ -15,6 +15,7 @@ Optional properties: - clocks : a list of phandle + clock specifier pairs - phys : phandle + phy specifier pair - phy-names : "usb" + - resets : phandle + reset specifier pair Example (Sequoia 440EPx): ehci@e0000300 { diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c index c7dd93aad20c..4ee67728e443 100644 --- a/drivers/usb/host/ehci-platform.c +++ b/drivers/usb/host/ehci-platform.c @@ -29,6 +29,7 @@ #include <linux/of.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> +#include <linux/reset.h> #include <linux/usb.h> #include <linux/usb/hcd.h> #include <linux/usb/ehci_pdriver.h> @@ -41,6 +42,7 @@ struct ehci_platform_priv { struct clk *clks[EHCI_MAX_CLKS]; + struct reset_control *rst; struct phy *phy; }; @@ -84,10 +86,16 @@ static int ehci_platform_power_on(struct platform_device *dev) goto err_disable_clks; } + if (priv->rst) { + ret = reset_control_deassert(priv->rst); + if (ret) + goto err_disable_clks; + } + if (priv->phy) { ret = phy_init(priv->phy); if (ret) - goto err_disable_clks; + goto err_assert_rst; ret = phy_power_on(priv->phy); if (ret) @@ -98,6 +106,9 @@ static int ehci_platform_power_on(struct platform_device *dev) err_exit_phy: phy_exit(priv->phy); +err_assert_rst: + if (priv->rst) + reset_control_assert(priv->rst); err_disable_clks: while (--clk >= 0) clk_disable_unprepare(priv->clks[clk]); @@ -119,6 +130,9 @@ static void ehci_platform_power_off(struct platform_device *dev) for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--) if (priv->clks[clk]) clk_disable_unprepare(priv->clks[clk]); + + if (priv->rst) + reset_control_assert(priv->rst); } static struct hc_driver __read_mostly ehci_platform_hc_driver; @@ -206,6 +220,15 @@ static int ehci_platform_probe(struct platform_device *dev) break; } } + + priv->rst = devm_reset_control_get_optional(&dev->dev, + NULL); + if (IS_ERR(priv->rst)) { + err = PTR_ERR(priv->rst); + if (err == -EPROBE_DEFER) + goto err_put_clks; + priv->rst = NULL; + } } if (pdata->big_endian_desc)