Linux-USB Archive on lore.kernel.org
 help / color / 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; 4+ 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	[flat|nested] 4+ 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; 4+ 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] 4+ 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; 4+ 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] 4+ 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
  1 sibling, 0 replies; 4+ 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] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ 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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git