* [PATCH] usb: host: xhci-tegra: Correct phy enable sequence @ 2019-11-04 9:24 Nagarjuna Kristam 2019-11-04 9:52 ` Thierry Reding 2019-11-07 15:32 ` Thierry Reding 0 siblings, 2 replies; 6+ messages in thread From: Nagarjuna Kristam @ 2019-11-04 9:24 UTC (permalink / raw) To: gregkh, thierry.reding, jonathanh Cc: linux-usb, Nagarjuna Kristam, Jui Chang Kuo XUSB phy needs to be enabled before un-powergating the power partitions. However in the current sequence, it happens opposite. Correct the phy enable and powergating partition sequence to avoid any boot hangs. Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com> Signed-off-by: Jui Chang Kuo <jckuo@nvidia.com> --- drivers/usb/host/xhci-tegra.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index 540b47a..bf90654 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -763,7 +763,6 @@ static int tegra_xusb_runtime_suspend(struct device *dev) { struct tegra_xusb *tegra = dev_get_drvdata(dev); - tegra_xusb_phy_disable(tegra); regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies); tegra_xusb_clk_disable(tegra); @@ -787,16 +786,8 @@ static int tegra_xusb_runtime_resume(struct device *dev) goto disable_clk; } - err = tegra_xusb_phy_enable(tegra); - if (err < 0) { - dev_err(dev, "failed to enable PHYs: %d\n", err); - goto disable_regulator; - } - return 0; -disable_regulator: - regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies); disable_clk: tegra_xusb_clk_disable(tegra); return err; @@ -1188,6 +1179,12 @@ static int tegra_xusb_probe(struct platform_device *pdev) */ platform_set_drvdata(pdev, tegra); + err = tegra_xusb_phy_enable(tegra); + if (err < 0) { + dev_err(&pdev->dev, "failed to enable PHYs: %d\n", err); + goto put_hcd; + } + pm_runtime_enable(&pdev->dev); if (pm_runtime_enabled(&pdev->dev)) err = pm_runtime_get_sync(&pdev->dev); @@ -1196,7 +1193,7 @@ static int tegra_xusb_probe(struct platform_device *pdev) if (err < 0) { dev_err(&pdev->dev, "failed to enable device: %d\n", err); - goto disable_rpm; + goto disable_phy; } tegra_xusb_config(tegra, regs); @@ -1282,9 +1279,11 @@ static int tegra_xusb_probe(struct platform_device *pdev) put_rpm: if (!pm_runtime_status_suspended(&pdev->dev)) tegra_xusb_runtime_suspend(&pdev->dev); -disable_rpm: - pm_runtime_disable(&pdev->dev); +put_hcd: usb_put_hcd(tegra->hcd); +disable_phy: + tegra_xusb_phy_disable(tegra); + pm_runtime_disable(&pdev->dev); put_powerdomains: if (!of_property_read_bool(pdev->dev.of_node, "power-domains")) { tegra_powergate_power_off(TEGRA_POWERGATE_XUSBC); @@ -1321,6 +1320,8 @@ static int tegra_xusb_remove(struct platform_device *pdev) tegra_xusb_powerdomain_remove(&pdev->dev, tegra); } + tegra_xusb_phy_disable(tegra); + tegra_xusb_padctl_put(tegra->padctl); return 0; -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: host: xhci-tegra: Correct phy enable sequence 2019-11-04 9:24 [PATCH] usb: host: xhci-tegra: Correct phy enable sequence Nagarjuna Kristam @ 2019-11-04 9:52 ` Thierry Reding 2019-11-05 8:42 ` Nagarjuna Kristam 2019-11-07 15:32 ` Thierry Reding 1 sibling, 1 reply; 6+ messages in thread From: Thierry Reding @ 2019-11-04 9:52 UTC (permalink / raw) To: Nagarjuna Kristam; +Cc: gregkh, jonathanh, linux-usb, Jui Chang Kuo [-- Attachment #1: Type: text/plain, Size: 4031 bytes --] On Mon, Nov 04, 2019 at 02:54:30PM +0530, Nagarjuna Kristam wrote: > XUSB phy needs to be enabled before un-powergating the power partitions. > However in the current sequence, it happens opposite. Correct the phy > enable and powergating partition sequence to avoid any boot hangs. > > Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com> > Signed-off-by: Jui Chang Kuo <jckuo@nvidia.com> > --- > drivers/usb/host/xhci-tegra.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) Do you know what the amount of power is that the PHYs drain? We're now no longer able to power off the PHYs when the XHCI controller goes into runtime suspend. That's a little unfortunate, but given the powergate sequence, there's no easy way around that. If the amount of power we're now consuming the idle state is significant enough, it may be an incentive to look into a way that allows us to save it. If it's insignificant, it might not be worth looking into it much further. My understanding is that the only reason we can't do this right now is because the generic power domains are always enabled before the device's ->runtime_resume() is called and disabled after ->runtime_suspend() is called. So one possibility would be to add a mechanism to mark a device as wanting explicit control over the generic power domain. That way we could order the power domain vs. PHY in a way that's compatible with the powergate sequence. Again, it may not be worth the extra effort, but without numbers that's a difficult call to make. Thierry > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index 540b47a..bf90654 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -763,7 +763,6 @@ static int tegra_xusb_runtime_suspend(struct device *dev) > { > struct tegra_xusb *tegra = dev_get_drvdata(dev); > > - tegra_xusb_phy_disable(tegra); > regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies); > tegra_xusb_clk_disable(tegra); > > @@ -787,16 +786,8 @@ static int tegra_xusb_runtime_resume(struct device *dev) > goto disable_clk; > } > > - err = tegra_xusb_phy_enable(tegra); > - if (err < 0) { > - dev_err(dev, "failed to enable PHYs: %d\n", err); > - goto disable_regulator; > - } > - > return 0; > > -disable_regulator: > - regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies); > disable_clk: > tegra_xusb_clk_disable(tegra); > return err; > @@ -1188,6 +1179,12 @@ static int tegra_xusb_probe(struct platform_device *pdev) > */ > platform_set_drvdata(pdev, tegra); > > + err = tegra_xusb_phy_enable(tegra); > + if (err < 0) { > + dev_err(&pdev->dev, "failed to enable PHYs: %d\n", err); > + goto put_hcd; > + } > + > pm_runtime_enable(&pdev->dev); > if (pm_runtime_enabled(&pdev->dev)) > err = pm_runtime_get_sync(&pdev->dev); > @@ -1196,7 +1193,7 @@ static int tegra_xusb_probe(struct platform_device *pdev) > > if (err < 0) { > dev_err(&pdev->dev, "failed to enable device: %d\n", err); > - goto disable_rpm; > + goto disable_phy; > } > > tegra_xusb_config(tegra, regs); > @@ -1282,9 +1279,11 @@ static int tegra_xusb_probe(struct platform_device *pdev) > put_rpm: > if (!pm_runtime_status_suspended(&pdev->dev)) > tegra_xusb_runtime_suspend(&pdev->dev); > -disable_rpm: > - pm_runtime_disable(&pdev->dev); > +put_hcd: > usb_put_hcd(tegra->hcd); > +disable_phy: > + tegra_xusb_phy_disable(tegra); > + pm_runtime_disable(&pdev->dev); > put_powerdomains: > if (!of_property_read_bool(pdev->dev.of_node, "power-domains")) { > tegra_powergate_power_off(TEGRA_POWERGATE_XUSBC); > @@ -1321,6 +1320,8 @@ static int tegra_xusb_remove(struct platform_device *pdev) > tegra_xusb_powerdomain_remove(&pdev->dev, tegra); > } > > + tegra_xusb_phy_disable(tegra); > + > tegra_xusb_padctl_put(tegra->padctl); > > return 0; > -- > 2.7.4 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: host: xhci-tegra: Correct phy enable sequence 2019-11-04 9:52 ` Thierry Reding @ 2019-11-05 8:42 ` Nagarjuna Kristam 0 siblings, 0 replies; 6+ messages in thread From: Nagarjuna Kristam @ 2019-11-05 8:42 UTC (permalink / raw) To: Thierry Reding; +Cc: gregkh, jonathanh, linux-usb, Jui Chang Kuo On 04-11-2019 15:22, Thierry Reding wrote: > On Mon, Nov 04, 2019 at 02:54:30PM +0530, Nagarjuna Kristam wrote: >> XUSB phy needs to be enabled before un-powergating the power partitions. >> However in the current sequence, it happens opposite. Correct the phy >> enable and powergating partition sequence to avoid any boot hangs. >> >> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com> >> Signed-off-by: Jui Chang Kuo <jckuo@nvidia.com> >> --- >> drivers/usb/host/xhci-tegra.c | 25 +++++++++++++------------ >> 1 file changed, 13 insertions(+), 12 deletions(-) > > Do you know what the amount of power is that the PHYs drain? We're now > no longer able to power off the PHYs when the XHCI controller goes into > runtime suspend. That's a little unfortunate, but given the powergate > sequence, there's no easy way around that. > > If the amount of power we're now consuming the idle state is significant > enough, it may be an incentive to look into a way that allows us to save > it. If it's insignificant, it might not be worth looking into it much > further. > > My understanding is that the only reason we can't do this right now is > because the generic power domains are always enabled before the device's > ->runtime_resume() is called and disabled after ->runtime_suspend() is > called. So one possibility would be to add a mechanism to mark a device > as wanting explicit control over the generic power domain. That way we > could order the power domain vs. PHY in a way that's compatible with the > powergate sequence. > > Again, it may not be worth the extra effort, but without numbers that's > a difficult call to make. > > Thierry > Thierry, For USB host driver, power savings are done via ELPG feature support. They are currently handled in https://patchwork.kernel.org/cover/10994599/ It is required to have USB phy always powered on to have host mode functionality. Performing run-time suspend/resume with phy enable breaks host mode functionality. In the current version of the driver, even though phy enable/disable calls are part of pm runtime, they are controlled only from _probe and _remove only, which is exactly same sequence used in current change, only order reversed. Hence, current changes does not cause any change in power consumption of phy lines. Thanks, Nagarjuna >> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c >> index 540b47a..bf90654 100644 >> --- a/drivers/usb/host/xhci-tegra.c >> +++ b/drivers/usb/host/xhci-tegra.c >> @@ -763,7 +763,6 @@ static int tegra_xusb_runtime_suspend(struct device *dev) >> { >> struct tegra_xusb *tegra = dev_get_drvdata(dev); >> >> - tegra_xusb_phy_disable(tegra); >> regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies); >> tegra_xusb_clk_disable(tegra); >> >> @@ -787,16 +786,8 @@ static int tegra_xusb_runtime_resume(struct device *dev) >> goto disable_clk; >> } >> >> - err = tegra_xusb_phy_enable(tegra); >> - if (err < 0) { >> - dev_err(dev, "failed to enable PHYs: %d\n", err); >> - goto disable_regulator; >> - } >> - >> return 0; >> >> -disable_regulator: >> - regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies); >> disable_clk: >> tegra_xusb_clk_disable(tegra); >> return err; >> @@ -1188,6 +1179,12 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> */ >> platform_set_drvdata(pdev, tegra); >> >> + err = tegra_xusb_phy_enable(tegra); >> + if (err < 0) { >> + dev_err(&pdev->dev, "failed to enable PHYs: %d\n", err); >> + goto put_hcd; >> + } >> + >> pm_runtime_enable(&pdev->dev); >> if (pm_runtime_enabled(&pdev->dev)) >> err = pm_runtime_get_sync(&pdev->dev); >> @@ -1196,7 +1193,7 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> >> if (err < 0) { >> dev_err(&pdev->dev, "failed to enable device: %d\n", err); >> - goto disable_rpm; >> + goto disable_phy; >> } >> >> tegra_xusb_config(tegra, regs); >> @@ -1282,9 +1279,11 @@ static int tegra_xusb_probe(struct platform_device *pdev) >> put_rpm: >> if (!pm_runtime_status_suspended(&pdev->dev)) >> tegra_xusb_runtime_suspend(&pdev->dev); >> -disable_rpm: >> - pm_runtime_disable(&pdev->dev); >> +put_hcd: >> usb_put_hcd(tegra->hcd); >> +disable_phy: >> + tegra_xusb_phy_disable(tegra); >> + pm_runtime_disable(&pdev->dev); >> put_powerdomains: >> if (!of_property_read_bool(pdev->dev.of_node, "power-domains")) { >> tegra_powergate_power_off(TEGRA_POWERGATE_XUSBC); >> @@ -1321,6 +1320,8 @@ static int tegra_xusb_remove(struct platform_device *pdev) >> tegra_xusb_powerdomain_remove(&pdev->dev, tegra); >> } >> >> + tegra_xusb_phy_disable(tegra); >> + >> tegra_xusb_padctl_put(tegra->padctl); >> >> return 0; >> -- >> 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: host: xhci-tegra: Correct phy enable sequence 2019-11-04 9:24 [PATCH] usb: host: xhci-tegra: Correct phy enable sequence Nagarjuna Kristam 2019-11-04 9:52 ` Thierry Reding @ 2019-11-07 15:32 ` Thierry Reding 2019-11-18 9:49 ` Jon Hunter 1 sibling, 1 reply; 6+ messages in thread From: Thierry Reding @ 2019-11-07 15:32 UTC (permalink / raw) To: Nagarjuna Kristam; +Cc: gregkh, jonathanh, linux-usb, Jui Chang Kuo [-- Attachment #1: Type: text/plain, Size: 575 bytes --] On Mon, Nov 04, 2019 at 02:54:30PM +0530, Nagarjuna Kristam wrote: > XUSB phy needs to be enabled before un-powergating the power partitions. > However in the current sequence, it happens opposite. Correct the phy > enable and powergating partition sequence to avoid any boot hangs. > > Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com> > Signed-off-by: Jui Chang Kuo <jckuo@nvidia.com> > --- > drivers/usb/host/xhci-tegra.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: host: xhci-tegra: Correct phy enable sequence 2019-11-07 15:32 ` Thierry Reding @ 2019-11-18 9:49 ` Jon Hunter 2019-11-18 11:30 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Jon Hunter @ 2019-11-18 9:49 UTC (permalink / raw) To: Thierry Reding, Nagarjuna Kristam, gregkh Cc: linux-usb, Jui Chang Kuo, linux-tegra Hi Greg, On 07/11/2019 15:32, Thierry Reding wrote: > On Mon, Nov 04, 2019 at 02:54:30PM +0530, Nagarjuna Kristam wrote: >> XUSB phy needs to be enabled before un-powergating the power partitions. >> However in the current sequence, it happens opposite. Correct the phy >> enable and powergating partition sequence to avoid any boot hangs. >> >> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com> >> Signed-off-by: Jui Chang Kuo <jckuo@nvidia.com> >> --- >> drivers/usb/host/xhci-tegra.c | 25 +++++++++++++------------ >> 1 file changed, 13 insertions(+), 12 deletions(-) > > Acked-by: Thierry Reding <treding@nvidia.com> Let me know if you can pick this one up? This is fixing a boot regression on Tegra210. Tested-by: Jon Hunter <jonathanh@nvidia.com> Cheers Jon -- nvpublic ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: host: xhci-tegra: Correct phy enable sequence 2019-11-18 9:49 ` Jon Hunter @ 2019-11-18 11:30 ` Greg KH 0 siblings, 0 replies; 6+ messages in thread From: Greg KH @ 2019-11-18 11:30 UTC (permalink / raw) To: Jon Hunter Cc: Thierry Reding, Nagarjuna Kristam, linux-usb, Jui Chang Kuo, linux-tegra On Mon, Nov 18, 2019 at 09:49:52AM +0000, Jon Hunter wrote: > Hi Greg, > > On 07/11/2019 15:32, Thierry Reding wrote: > > On Mon, Nov 04, 2019 at 02:54:30PM +0530, Nagarjuna Kristam wrote: > >> XUSB phy needs to be enabled before un-powergating the power partitions. > >> However in the current sequence, it happens opposite. Correct the phy > >> enable and powergating partition sequence to avoid any boot hangs. > >> > >> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com> > >> Signed-off-by: Jui Chang Kuo <jckuo@nvidia.com> > >> --- > >> drivers/usb/host/xhci-tegra.c | 25 +++++++++++++------------ > >> 1 file changed, 13 insertions(+), 12 deletions(-) > > > > Acked-by: Thierry Reding <treding@nvidia.com> > > Let me know if you can pick this one up? This is fixing a boot > regression on Tegra210. > > Tested-by: Jon Hunter <jonathanh@nvidia.com> Will do so now, sorry for the delay. greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-18 11:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-04 9:24 [PATCH] usb: host: xhci-tegra: Correct phy enable sequence Nagarjuna Kristam 2019-11-04 9:52 ` Thierry Reding 2019-11-05 8:42 ` Nagarjuna Kristam 2019-11-07 15:32 ` Thierry Reding 2019-11-18 9:49 ` Jon Hunter 2019-11-18 11:30 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).