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@xxxxxxxxxx> >> Signed-off-by: Jui Chang Kuo <jckuo@xxxxxxxxxx> >> --- >> 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
