| Message ID | 1572859470-7823-1-git-send-email-nkristam@nvidia.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6351653febbb784d86fdf83afe41f7523a61b392 |
| Headers | show |
| Series | usb: host: xhci-tegra: Correct phy enable sequence | expand |
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 >
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
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>
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
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
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;