Re: [PATCH] usb: host: xhci-tegra: Correct phy enable sequence

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]