linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).