diff mbox

[v3,1/2] usb: host: ehci-tegra: Grab the correct UTMI pads reset

Message ID 1462372800-30900-1-git-send-email-thierry.reding@gmail.com
State Superseded, archived
Headers show

Commit Message

Thierry Reding May 4, 2016, 2:39 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

There are three EHCI controllers on Tegra SoCs, each with its own reset
line. However, the first controller contains a set of UTMI configuration
registers that are shared with its siblings. These registers will only
be reset as part of the first controller's reset. For proper operation
it must be ensured that the UTMI configuration registers are reset
before any of the EHCI controllers are enabled, irrespective of the
probe order.

Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
broken USB") introduced code that ensures the first controller is always
reset before setting up any of the controllers, and is never again reset
afterwards.

This code, however, grabs the wrong reset. Each EHCI controller has two
reset controls attached: 1) the USB controller reset and 2) the UTMI
pads reset (really the first controller's reset). In order to reset the
UTMI pads registers the code must grab the second reset, but instead it
grabbing the first.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Stephen, Alex, Jon, have you ever encountered cases where UTMI might not
have worked correctly? It seems that this code was pulsing the wrong
reset line and therefore the UTMI pads would never be reset unless the
first USB controller was probed before all others. I've never seen any
such problems myself, so I'm unsure about whether it's worth Cc'ing the
patch to stable@vger.kernel.org.

 drivers/usb/host/ehci-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg Kroah-Hartman May 4, 2016, 2:57 p.m. UTC | #1 
On Wed, May 04, 2016 at 04:39:59PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> There are three EHCI controllers on Tegra SoCs, each with its own reset
> line. However, the first controller contains a set of UTMI configuration
> registers that are shared with its siblings. These registers will only
> be reset as part of the first controller's reset. For proper operation
> it must be ensured that the UTMI configuration registers are reset
> before any of the EHCI controllers are enabled, irrespective of the
> probe order.
> 
> Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
> broken USB") introduced code that ensures the first controller is always
> reset before setting up any of the controllers, and is never again reset
> afterwards.
> 
> This code, however, grabs the wrong reset. Each EHCI controller has two
> reset controls attached: 1) the USB controller reset and 2) the UTMI
> pads reset (really the first controller's reset). In order to reset the
> UTMI pads registers the code must grab the second reset, but instead it
> grabbing the first.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Any reason you don't want this backported to stable kernels?

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding May 4, 2016, 3:26 p.m. UTC | #2 
On Wed, May 04, 2016 at 07:57:10AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 04, 2016 at 04:39:59PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > There are three EHCI controllers on Tegra SoCs, each with its own reset
> > line. However, the first controller contains a set of UTMI configuration
> > registers that are shared with its siblings. These registers will only
> > be reset as part of the first controller's reset. For proper operation
> > it must be ensured that the UTMI configuration registers are reset
> > before any of the EHCI controllers are enabled, irrespective of the
> > probe order.
> > 
> > Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
> > broken USB") introduced code that ensures the first controller is always
> > reset before setting up any of the controllers, and is never again reset
> > afterwards.
> > 
> > This code, however, grabs the wrong reset. Each EHCI controller has two
> > reset controls attached: 1) the USB controller reset and 2) the UTMI
> > pads reset (really the first controller's reset). In order to reset the
> > UTMI pads registers the code must grab the second reset, but instead it
> > grabbing the first.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Any reason you don't want this backported to stable kernels?

There's a brief note below the commit message that explains why I'm not
sure if there's a need to backport:

> Stephen, Alex, Jon, have you ever encountered cases where UTMI might not
> have worked correctly? It seems that this code was pulsing the wrong
> reset line and therefore the UTMI pads would never be reset unless the
> first USB controller was probed before all others. I've never seen any
> such problems myself, so I'm unsure about whether it's worth Cc'ing the
> patch to stable@vger.kernel.org.

So the bottom line is that I have no evidence that this fixes any real
issue, hence I'm not sure if it's worth bothering the stable kernel
maintainers with it.

While at it, adding Tuomas who wrote the original probe order fix.
Tuomas, does this patch look correct to you? Here's the patch in full if
you don't have it in your inbox:

	http://patchwork.ozlabs.org/patch/618488/

Thanks,
Thierry
Stephen Warren May 4, 2016, 5:14 p.m. UTC | #3 
On 05/04/2016 08:39 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> There are three EHCI controllers on Tegra SoCs, each with its own reset
> line. However, the first controller contains a set of UTMI configuration
> registers that are shared with its siblings. These registers will only
> be reset as part of the first controller's reset. For proper operation
> it must be ensured that the UTMI configuration registers are reset
> before any of the EHCI controllers are enabled, irrespective of the
> probe order.
>
> Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
> broken USB") introduced code that ensures the first controller is always
> reset before setting up any of the controllers, and is never again reset
> afterwards.
>
> This code, however, grabs the wrong reset. Each EHCI controller has two
> reset controls attached: 1) the USB controller reset and 2) the UTMI
> pads reset (really the first controller's reset). In order to reset the
> UTMI pads registers the code must grab the second reset, but instead it
> grabbing the first.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Stephen, Alex, Jon, have you ever encountered cases where UTMI might not
> have worked correctly? It seems that this code was pulsing the wrong
> reset line and therefore the UTMI pads would never be reset unless the
> first USB controller was probed before all others. I've never seen any
> such problems myself, so I'm unsure about whether it's worth Cc'ing the
> patch to stable@vger.kernel.org.

I don't think I recall seeing USB issues like that, although I don't use 
USB a huge amount. Perhaps the issue just never happens because we 
always have USB1 enabled, and it's physically present in the DTB first, 
so it always happens to get probed first?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding May 4, 2016, 8:30 p.m. UTC | #4 
On Wed, May 04, 2016 at 11:14:50AM -0600, Stephen Warren wrote:
> On 05/04/2016 08:39 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > There are three EHCI controllers on Tegra SoCs, each with its own reset
> > line. However, the first controller contains a set of UTMI configuration
> > registers that are shared with its siblings. These registers will only
> > be reset as part of the first controller's reset. For proper operation
> > it must be ensured that the UTMI configuration registers are reset
> > before any of the EHCI controllers are enabled, irrespective of the
> > probe order.
> > 
> > Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
> > broken USB") introduced code that ensures the first controller is always
> > reset before setting up any of the controllers, and is never again reset
> > afterwards.
> > 
> > This code, however, grabs the wrong reset. Each EHCI controller has two
> > reset controls attached: 1) the USB controller reset and 2) the UTMI
> > pads reset (really the first controller's reset). In order to reset the
> > UTMI pads registers the code must grab the second reset, but instead it
> > grabbing the first.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Stephen, Alex, Jon, have you ever encountered cases where UTMI might not
> > have worked correctly? It seems that this code was pulsing the wrong
> > reset line and therefore the UTMI pads would never be reset unless the
> > first USB controller was probed before all others. I've never seen any
> > such problems myself, so I'm unsure about whether it's worth Cc'ing the
> > patch to stable@vger.kernel.org.
> 
> I don't think I recall seeing USB issues like that, although I don't use USB
> a huge amount. Perhaps the issue just never happens because we always have
> USB1 enabled, and it's physically present in the DTB first, so it always
> happens to get probed first?

Actually for Jetson TK1 we don't enable USB1. It's interesting because
v1 of patch 2/2 only fixed the issue for device where USB1 was indeed
enabled and probed first (I tested on TrimSlice). Running these fixes
through EIMT I noticed that it didn't fix it in the general case and I
was still seeing the warning on Jetson TK1 for example.

That's in fact what tipped me off about the consumer name, because I was
seeing reset 58 (USB2) being requested twice.

Thierry
Jon Hunter May 5, 2016, 7:39 a.m. UTC | #5 
On 04/05/16 15:39, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> There are three EHCI controllers on Tegra SoCs, each with its own reset
> line. However, the first controller contains a set of UTMI configuration
> registers that are shared with its siblings. These registers will only
> be reset as part of the first controller's reset. For proper operation
> it must be ensured that the UTMI configuration registers are reset
> before any of the EHCI controllers are enabled, irrespective of the
> probe order.
> 
> Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
> broken USB") introduced code that ensures the first controller is always
> reset before setting up any of the controllers, and is never again reset
> afterwards.
> 
> This code, however, grabs the wrong reset. Each EHCI controller has two
> reset controls attached: 1) the USB controller reset and 2) the UTMI
> pads reset (really the first controller's reset). In order to reset the
> UTMI pads registers the code must grab the second reset, but instead it
> grabbing the first.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Stephen, Alex, Jon, have you ever encountered cases where UTMI might not
> have worked correctly? It seems that this code was pulsing the wrong
> reset line and therefore the UTMI pads would never be reset unless the
> first USB controller was probed before all others. I've never seen any
> such problems myself, so I'm unsure about whether it's worth Cc'ing the
> patch to stable@vger.kernel.org.
> 
>  drivers/usb/host/ehci-tegra.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 4031b372008e..c1c1024a054c 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -89,7 +89,7 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  	if (!usb1_reset_attempted) {
>  		struct reset_control *usb1_reset;
>  
> -		usb1_reset = of_reset_control_get(phy_np, "usb");
> +		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>  		if (IS_ERR(usb1_reset)) {
>  			dev_warn(&pdev->dev,
>  				 "can't get utmi-pads reset from the PHY\n");
> 

I have not seen any issues either, but may be we were getting lucky. The
change makes sense to me.

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tuomas Tynkkynen May 5, 2016, 4:05 p.m. UTC | #6 
On 05/04/2016 06:26 PM, Thierry Reding wrote:
> On Wed, May 04, 2016 at 07:57:10AM -0700, Greg Kroah-Hartman wrote:
>> On Wed, May 04, 2016 at 04:39:59PM +0200, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> There are three EHCI controllers on Tegra SoCs, each with its own reset
>>> line. However, the first controller contains a set of UTMI configuration
>>> registers that are shared with its siblings. These registers will only
>>> be reset as part of the first controller's reset. For proper operation
>>> it must be ensured that the UTMI configuration registers are reset
>>> before any of the EHCI controllers are enabled, irrespective of the
>>> probe order.
>>>
>>> Commit a47cc24cd1e5 ("USB: EHCI: tegra: Fix probe order issue leading to
>>> broken USB") introduced code that ensures the first controller is always
>>> reset before setting up any of the controllers, and is never again reset
>>> afterwards.
>>>
>>> This code, however, grabs the wrong reset. Each EHCI controller has two
>>> reset controls attached: 1) the USB controller reset and 2) the UTMI
>>> pads reset (really the first controller's reset). In order to reset the
>>> UTMI pads registers the code must grab the second reset, but instead it
>>> grabbing the first.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
...snip ...
>
> While at it, adding Tuomas who wrote the original probe order fix.
> Tuomas, does this patch look correct to you? Here's the patch in full if
> you don't have it in your inbox:
>
> 	http://patchwork.ozlabs.org/patch/618488/
>

D'oh! Yes, that patch looks correct.

- Tuomas

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 4031b372008e..c1c1024a054c 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -89,7 +89,7 @@  static int tegra_reset_usb_controller(struct platform_device *pdev)
 	if (!usb1_reset_attempted) {
 		struct reset_control *usb1_reset;
 
-		usb1_reset = of_reset_control_get(phy_np, "usb");
+		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
 		if (IS_ERR(usb1_reset)) {
 			dev_warn(&pdev->dev,
 				 "can't get utmi-pads reset from the PHY\n");