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

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

[Thierry Reding]

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");
-- 
2.8.0

[PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Starting with commit 0b52297f2288 ("reset: Add support for shared reset
controls") there is a reference count for reset control assertions. The
goal is to allow resets to be shared by multiple devices and an assert
will take effect only when all instances have asserted the reset.

In order to preserve backwards-compatibility, all reset controls become
exclusive by default. This is to ensure that reset_control_assert() can
immediately assert in hardware.

However, this new behaviour triggers the following warning in the EHCI
driver for Tegra:

[    3.365019] ------------[ cut here ]------------
[    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
[    3.382151] Modules linked in:
[    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
[    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
[    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
[    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
[    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
[    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
[    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
[    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
[    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
[    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
[    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
[    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
[    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
[    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
[    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
[    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
[    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
[    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---

The reason is that Tegra SoCs have three EHCI controllers, each with a
separate reset line. However the first controller contains UTMI pads
configuration registers that are shared with its siblings and that are
reset as part of the first controller's reset. There is special code in
the driver to assert and deassert this shared reset at probe time, and
it does so irrespective of which controller is probed first to ensure
that these shared registers are reset before any of the controllers are
initialized. Unfortunately this means that if the first controller gets
probed first, it will request its own reset line and will subsequently
request the same reset line again (temporarily) to perform the reset.
This used to work fine before the above-mentioned commit, but now
triggers the new WARN.

Work around this by making sure we reuse the controller's reset if the
controller happens to be the first controller.

Cc: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- reword commit message to more accurately describe the hardware design

Changes in v2:
- restore has_utmi_pad_registers condition (Alan Stern)

 drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index c1c1024a054c..8396b622f238 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct tegra_ehci_hcd *tegra =
 		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
+	bool has_utmi_pad_registers = false;
 
 	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
 	if (!phy_np)
 		return -ENOENT;
 
+	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
+		has_utmi_pad_registers = true;
+
 	if (!usb1_reset_attempted) {
 		struct reset_control *usb1_reset;
 
-		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+		if (!has_utmi_pad_registers)
+			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+		else
+			usb1_reset = tegra->rst;
+
 		if (IS_ERR(usb1_reset)) {
 			dev_warn(&pdev->dev,
 				 "can't get utmi-pads reset from the PHY\n");
@@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
 			reset_control_deassert(usb1_reset);
 		}
 
-		reset_control_put(usb1_reset);
+		if (!has_utmi_pad_registers)
+			reset_control_put(usb1_reset);
+
 		usb1_reset_attempted = true;
 	}
 
-	if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
+	if (!has_utmi_pad_registers) {
 		reset_control_assert(tegra->rst);
 		udelay(1);
 		reset_control_deassert(tegra->rst);
-- 
2.8.0

[PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Starting with commit 0b52297f2288 ("reset: Add support for shared reset
controls") there is a reference count for reset control assertions. The
goal is to allow resets to be shared by multiple devices and an assert
will take effect only when all instances have asserted the reset.

In order to preserve backwards-compatibility, all reset controls become
exclusive by default. This is to ensure that reset_control_assert() can
immediately assert in hardware.

However, this new behaviour triggers the following warning in the EHCI
driver for Tegra:

[    3.365019] ------------[ cut here ]------------
[    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
[    3.382151] Modules linked in:
[    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
[    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
[    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
[    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
[    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
[    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
[    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
[    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
[    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
[    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
[    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
[    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
[    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
[    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
[    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
[    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
[    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
[    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---

The reason is that Tegra SoCs have three EHCI controllers, each with a
separate reset line. However the first controller contains UTMI pads
configuration registers that are shared with its siblings and that are
reset as part of the first controller's reset. There is special code in
the driver to assert and deassert this shared reset at probe time, and
it does so irrespective of which controller is probed first to ensure
that these shared registers are reset before any of the controllers are
initialized. Unfortunately this means that if the first controller gets
probed first, it will request its own reset line and will subsequently
request the same reset line again (temporarily) to perform the reset.
This used to work fine before the above-mentioned commit, but now
triggers the new WARN.

Work around this by making sure we reuse the controller's reset if the
controller happens to be the first controller.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- reword commit message to more accurately describe the hardware design

Changes in v2:
- restore has_utmi_pad_registers condition (Alan Stern)

 drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index c1c1024a054c..8396b622f238 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct tegra_ehci_hcd *tegra =
 		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
+	bool has_utmi_pad_registers = false;
 
 	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
 	if (!phy_np)
 		return -ENOENT;
 
+	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
+		has_utmi_pad_registers = true;
+
 	if (!usb1_reset_attempted) {
 		struct reset_control *usb1_reset;
 
-		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+		if (!has_utmi_pad_registers)
+			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
+		else
+			usb1_reset = tegra->rst;
+
 		if (IS_ERR(usb1_reset)) {
 			dev_warn(&pdev->dev,
 				 "can't get utmi-pads reset from the PHY\n");
@@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
 			reset_control_deassert(usb1_reset);
 		}
 
-		reset_control_put(usb1_reset);
+		if (!has_utmi_pad_registers)
+			reset_control_put(usb1_reset);
+
 		usb1_reset_attempted = true;
 	}
 
-	if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
+	if (!has_utmi_pad_registers) {
 		reset_control_assert(tegra->rst);
 		udelay(1);
 		reset_control_deassert(tegra->rst);
-- 
2.8.0

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

[Greg Kroah-Hartman]

On Wed, May 04, 2016 at 04:39:59PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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

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

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

[Greg Kroah-Hartman]

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?

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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-u79uwXL29TaiAVqoAR/hOA@public.gmane.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Stephen Warren]

On 05/04/2016 08:39 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 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-u79uwXL29TaiAVqoAR/hOA@public.gmane.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?

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

[Stephen Warren]

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?

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Philipp Zabel]

Hi Thierry,

Am Mittwoch, den 04.05.2016, 16:40 +0200 schrieb Thierry Reding:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
> 
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
> 
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
> 
> [    3.365019] ------------[ cut here ]------------
> [    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
> [    3.382151] Modules linked in:
> [    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
> [    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
> [    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
> [    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
> [    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
> [    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
> [    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
> [    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
> [    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
> [    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
> [    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
> [    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
> [    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
> [    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
> [    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
> [    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
> [    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
> [    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> 
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
> 
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.
> 
> Cc: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v3:
> - reword commit message to more accurately describe the hardware design

Reviewed-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

> Changes in v2:
> - restore has_utmi_pad_registers condition (Alan Stern)
> 
>  drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..8396b622f238 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>  	struct tegra_ehci_hcd *tegra =
>  		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> +	bool has_utmi_pad_registers = false;
>  
>  	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>  	if (!phy_np)
>  		return -ENOENT;
>  
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;
> +
>  	if (!usb1_reset_attempted) {
>  		struct reset_control *usb1_reset;
>  
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
> +
>  		if (IS_ERR(usb1_reset)) {
>  			dev_warn(&pdev->dev,
>  				 "can't get utmi-pads reset from the PHY\n");
> @@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  			reset_control_deassert(usb1_reset);
>  		}
>  
> -		reset_control_put(usb1_reset);
> +		if (!has_utmi_pad_registers)
> +			reset_control_put(usb1_reset);
> +

While you touch this, could you move the reset_control_put inside the
(!IS_ERR(usb1_reset)) section above, maybe in a separate patch?

We might start to warn in reset_control_put() if it is called with an
error value as parameter in the future.

>  		usb1_reset_attempted = true;
>  	}
>  
> -	if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
> +	if (!has_utmi_pad_registers) {
>  		reset_control_assert(tegra->rst);
>  		udelay(1);
>  		reset_control_deassert(tegra->rst);

regards
Philipp

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

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Philipp Zabel]

Hi Thierry,

Am Mittwoch, den 04.05.2016, 16:40 +0200 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
> 
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
> 
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
> 
> [    3.365019] ------------[ cut here ]------------
> [    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
> [    3.382151] Modules linked in:
> [    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
> [    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
> [    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
> [    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
> [    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
> [    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
> [    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
> [    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
> [    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
> [    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
> [    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
> [    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
> [    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
> [    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
> [    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
> [    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
> [    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
> [    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> 
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
> 
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v3:
> - reword commit message to more accurately describe the hardware design

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

> Changes in v2:
> - restore has_utmi_pad_registers condition (Alan Stern)
> 
>  drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..8396b622f238 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>  	struct tegra_ehci_hcd *tegra =
>  		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> +	bool has_utmi_pad_registers = false;
>  
>  	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>  	if (!phy_np)
>  		return -ENOENT;
>  
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;
> +
>  	if (!usb1_reset_attempted) {
>  		struct reset_control *usb1_reset;
>  
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
> +
>  		if (IS_ERR(usb1_reset)) {
>  			dev_warn(&pdev->dev,
>  				 "can't get utmi-pads reset from the PHY\n");
> @@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  			reset_control_deassert(usb1_reset);
>  		}
>  
> -		reset_control_put(usb1_reset);
> +		if (!has_utmi_pad_registers)
> +			reset_control_put(usb1_reset);
> +

While you touch this, could you move the reset_control_put inside the
(!IS_ERR(usb1_reset)) section above, maybe in a separate patch?

We might start to warn in reset_control_put() if it is called with an
error value as parameter in the future.

>  		usb1_reset_attempted = true;
>  	}
>  
> -	if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
> +	if (!has_utmi_pad_registers) {
>  		reset_control_assert(tegra->rst);
>  		udelay(1);
>  		reset_control_deassert(tegra->rst);

regards
Philipp

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Stephen Warren]

On 05/04/2016 08:40 AM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
>
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
>
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
...
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
>
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)

> +	bool has_utmi_pad_registers = false;
>
>   	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>   	if (!phy_np)
>   		return -ENOENT;
>
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;

Isn't that just:

has_utmi_pad_registers = of_property_read_bool(phy_np,
     "nvidia,has-utmi-pad-registers");

... and then you can remove " = false" from the declaration too?

>   	if (!usb1_reset_attempted) {
>   		struct reset_control *usb1_reset;
>
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
...
>   		usb1_reset_attempted = true;
>   	}

This is a pre-existing issue, but what happens if the probes for two USB 
controllers run in parallel; there seems to be missing locking related 
to testing/setting usb1_reset_attempted, which could cause multiple 
parallel attempts to get the "utmi-pads" reset object, which would 
presumably cause essentially the same issue this patch is solving in 
other cases?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Stephen Warren]

On 05/04/2016 08:40 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
>
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
>
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
...
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
>
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)

> +	bool has_utmi_pad_registers = false;
>
>   	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>   	if (!phy_np)
>   		return -ENOENT;
>
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;

Isn't that just:

has_utmi_pad_registers = of_property_read_bool(phy_np,
     "nvidia,has-utmi-pad-registers");

... and then you can remove " = false" from the declaration too?

>   	if (!usb1_reset_attempted) {
>   		struct reset_control *usb1_reset;
>
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
...
>   		usb1_reset_attempted = true;
>   	}

This is a pre-existing issue, but what happens if the probes for two USB 
controllers run in parallel; there seems to be missing locking related 
to testing/setting usb1_reset_attempted, which could cause multiple 
parallel attempts to get the "utmi-pads" reset object, which would 
presumably cause essentially the same issue this patch is solving in 
other cases?

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6197 bytes --]

On Wed, May 04, 2016 at 07:22:54PM +0200, Philipp Zabel wrote:
> Hi Thierry,
> 
> Am Mittwoch, den 04.05.2016, 16:40 +0200 schrieb Thierry Reding:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> > controls") there is a reference count for reset control assertions. The
> > goal is to allow resets to be shared by multiple devices and an assert
> > will take effect only when all instances have asserted the reset.
> > 
> > In order to preserve backwards-compatibility, all reset controls become
> > exclusive by default. This is to ensure that reset_control_assert() can
> > immediately assert in hardware.
> > 
> > However, this new behaviour triggers the following warning in the EHCI
> > driver for Tegra:
> > 
> > [    3.365019] ------------[ cut here ]------------
> > [    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
> > [    3.382151] Modules linked in:
> > [    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
> > [    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> > [    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
> > [    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
> > [    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
> > [    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
> > [    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
> > [    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
> > [    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
> > [    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
> > [    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
> > [    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
> > [    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
> > [    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
> > [    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
> > [    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
> > [    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
> > [    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
> > [    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> > 
> > The reason is that Tegra SoCs have three EHCI controllers, each with a
> > separate reset line. However the first controller contains UTMI pads
> > configuration registers that are shared with its siblings and that are
> > reset as part of the first controller's reset. There is special code in
> > the driver to assert and deassert this shared reset at probe time, and
> > it does so irrespective of which controller is probed first to ensure
> > that these shared registers are reset before any of the controllers are
> > initialized. Unfortunately this means that if the first controller gets
> > probed first, it will request its own reset line and will subsequently
> > request the same reset line again (temporarily) to perform the reset.
> > This used to work fine before the above-mentioned commit, but now
> > triggers the new WARN.
> > 
> > Work around this by making sure we reuse the controller's reset if the
> > controller happens to be the first controller.
> > 
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v3:
> > - reword commit message to more accurately describe the hardware design
> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks.

> > Changes in v2:
> > - restore has_utmi_pad_registers condition (Alan Stern)
> > 
> >  drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> > index c1c1024a054c..8396b622f238 100644
> > --- a/drivers/usb/host/ehci-tegra.c
> > +++ b/drivers/usb/host/ehci-tegra.c
> > @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
> >  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
> >  	struct tegra_ehci_hcd *tegra =
> >  		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> > +	bool has_utmi_pad_registers = false;
> >  
> >  	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
> >  	if (!phy_np)
> >  		return -ENOENT;
> >  
> > +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> > +		has_utmi_pad_registers = true;
> > +
> >  	if (!usb1_reset_attempted) {
> >  		struct reset_control *usb1_reset;
> >  
> > -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> > +		if (!has_utmi_pad_registers)
> > +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> > +		else
> > +			usb1_reset = tegra->rst;
> > +
> >  		if (IS_ERR(usb1_reset)) {
> >  			dev_warn(&pdev->dev,
> >  				 "can't get utmi-pads reset from the PHY\n");
> > @@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
> >  			reset_control_deassert(usb1_reset);
> >  		}
> >  
> > -		reset_control_put(usb1_reset);
> > +		if (!has_utmi_pad_registers)
> > +			reset_control_put(usb1_reset);
> > +
> 
> While you touch this, could you move the reset_control_put inside the
> (!IS_ERR(usb1_reset)) section above, maybe in a separate patch?
> 
> We might start to warn in reset_control_put() if it is called with an
> error value as parameter in the future.

Good point. I hadn't noticed that this could potentially be called with
a ERR_PTR()-encoded error code.

Fixed in v4.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3730 bytes --]

On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
> On 05/04/2016 08:40 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> > controls") there is a reference count for reset control assertions. The
> > goal is to allow resets to be shared by multiple devices and an assert
> > will take effect only when all instances have asserted the reset.
> > 
> > In order to preserve backwards-compatibility, all reset controls become
> > exclusive by default. This is to ensure that reset_control_assert() can
> > immediately assert in hardware.
> > 
> > However, this new behaviour triggers the following warning in the EHCI
> > driver for Tegra:
> ...
> > The reason is that Tegra SoCs have three EHCI controllers, each with a
> > separate reset line. However the first controller contains UTMI pads
> > configuration registers that are shared with its siblings and that are
> > reset as part of the first controller's reset. There is special code in
> > the driver to assert and deassert this shared reset at probe time, and
> > it does so irrespective of which controller is probed first to ensure
> > that these shared registers are reset before any of the controllers are
> > initialized. Unfortunately this means that if the first controller gets
> > probed first, it will request its own reset line and will subsequently
> > request the same reset line again (temporarily) to perform the reset.
> > This used to work fine before the above-mentioned commit, but now
> > triggers the new WARN.
> > 
> > Work around this by making sure we reuse the controller's reset if the
> > controller happens to be the first controller.
> 
> > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> 
> > @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
> 
> > +	bool has_utmi_pad_registers = false;
> > 
> >   	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
> >   	if (!phy_np)
> >   		return -ENOENT;
> > 
> > +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> > +		has_utmi_pad_registers = true;
> 
> Isn't that just:
> 
> has_utmi_pad_registers = of_property_read_bool(phy_np,
>     "nvidia,has-utmi-pad-registers");
> 
> ... and then you can remove " = false" from the declaration too?

Yes. This is really only for aesthetics. The direct assignment doesn't
fit within 80 columns, and wrapping it looks ugly whichever way you do
it.

> >   	if (!usb1_reset_attempted) {
> >   		struct reset_control *usb1_reset;
> > 
> > -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> > +		if (!has_utmi_pad_registers)
> > +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> > +		else
> > +			usb1_reset = tegra->rst;
> ...
> >   		usb1_reset_attempted = true;
> >   	}
> 
> This is a pre-existing issue, but what happens if the probes for two USB
> controllers run in parallel; there seems to be missing locking related to
> testing/setting usb1_reset_attempted, which could cause multiple parallel
> attempts to get the "utmi-pads" reset object, which would presumably cause
> essentially the same issue this patch is solving in other cases?

Hah! Interestingly my initial attempt at fixing this was to introduce a
lock to serialize these, because I thought that was what was going on. I
don't think this function can ever run concurrently for different
devices because the driver core already serializes probes (unless a
driver specifically requests asynchronous probing, which this one
doesn't).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3701 bytes --]

On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
> On 05/04/2016 08:40 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> > controls") there is a reference count for reset control assertions. The
> > goal is to allow resets to be shared by multiple devices and an assert
> > will take effect only when all instances have asserted the reset.
> > 
> > In order to preserve backwards-compatibility, all reset controls become
> > exclusive by default. This is to ensure that reset_control_assert() can
> > immediately assert in hardware.
> > 
> > However, this new behaviour triggers the following warning in the EHCI
> > driver for Tegra:
> ...
> > The reason is that Tegra SoCs have three EHCI controllers, each with a
> > separate reset line. However the first controller contains UTMI pads
> > configuration registers that are shared with its siblings and that are
> > reset as part of the first controller's reset. There is special code in
> > the driver to assert and deassert this shared reset at probe time, and
> > it does so irrespective of which controller is probed first to ensure
> > that these shared registers are reset before any of the controllers are
> > initialized. Unfortunately this means that if the first controller gets
> > probed first, it will request its own reset line and will subsequently
> > request the same reset line again (temporarily) to perform the reset.
> > This used to work fine before the above-mentioned commit, but now
> > triggers the new WARN.
> > 
> > Work around this by making sure we reuse the controller's reset if the
> > controller happens to be the first controller.
> 
> > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> 
> > @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
> 
> > +	bool has_utmi_pad_registers = false;
> > 
> >   	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
> >   	if (!phy_np)
> >   		return -ENOENT;
> > 
> > +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> > +		has_utmi_pad_registers = true;
> 
> Isn't that just:
> 
> has_utmi_pad_registers = of_property_read_bool(phy_np,
>     "nvidia,has-utmi-pad-registers");
> 
> ... and then you can remove " = false" from the declaration too?

Yes. This is really only for aesthetics. The direct assignment doesn't
fit within 80 columns, and wrapping it looks ugly whichever way you do
it.

> >   	if (!usb1_reset_attempted) {
> >   		struct reset_control *usb1_reset;
> > 
> > -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> > +		if (!has_utmi_pad_registers)
> > +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> > +		else
> > +			usb1_reset = tegra->rst;
> ...
> >   		usb1_reset_attempted = true;
> >   	}
> 
> This is a pre-existing issue, but what happens if the probes for two USB
> controllers run in parallel; there seems to be missing locking related to
> testing/setting usb1_reset_attempted, which could cause multiple parallel
> attempts to get the "utmi-pads" reset object, which would presumably cause
> essentially the same issue this patch is solving in other cases?

Hah! Interestingly my initial attempt at fixing this was to introduce a
lock to serialize these, because I thought that was what was going on. I
don't think this function can ever run concurrently for different
devices because the driver core already serializes probes (unless a
driver specifically requests asynchronous probing, which this one
doesn't).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > 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-u79uwXL29TaiAVqoAR/hOA@public.gmane.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jon Hunter]

On 04/05/16 15:39, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 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-u79uwXL29TaiAVqoAR/hOA@public.gmane.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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

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

[Jon Hunter]

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

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Hans de Goede]

Hi,

On 04-05-16 22:25, Thierry Reding wrote:
> On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
>> On 05/04/2016 08:40 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
>>> controls") there is a reference count for reset control assertions. The
>>> goal is to allow resets to be shared by multiple devices and an assert
>>> will take effect only when all instances have asserted the reset.
>>>
>>> In order to preserve backwards-compatibility, all reset controls become
>>> exclusive by default. This is to ensure that reset_control_assert() can
>>> immediately assert in hardware.
>>>
>>> However, this new behaviour triggers the following warning in the EHCI
>>> driver for Tegra:
>> ...
>>> The reason is that Tegra SoCs have three EHCI controllers, each with a
>>> separate reset line. However the first controller contains UTMI pads
>>> configuration registers that are shared with its siblings and that are
>>> reset as part of the first controller's reset. There is special code in
>>> the driver to assert and deassert this shared reset at probe time, and
>>> it does so irrespective of which controller is probed first to ensure
>>> that these shared registers are reset before any of the controllers are
>>> initialized. Unfortunately this means that if the first controller gets
>>> probed first, it will request its own reset line and will subsequently
>>> request the same reset line again (temporarily) to perform the reset.
>>> This used to work fine before the above-mentioned commit, but now
>>> triggers the new WARN.
>>>
>>> Work around this by making sure we reuse the controller's reset if the
>>> controller happens to be the first controller.
>>
>>> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
>>
>>> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>>
>>> +	bool has_utmi_pad_registers = false;
>>>
>>>   	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>>>   	if (!phy_np)
>>>   		return -ENOENT;
>>>
>>> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
>>> +		has_utmi_pad_registers = true;
>>
>> Isn't that just:
>>
>> has_utmi_pad_registers = of_property_read_bool(phy_np,
>>     "nvidia,has-utmi-pad-registers");
>>
>> ... and then you can remove " = false" from the declaration too?
>
> Yes. This is really only for aesthetics. The direct assignment doesn't
> fit within 80 columns, and wrapping it looks ugly whichever way you do
> it.
>
>>>   	if (!usb1_reset_attempted) {
>>>   		struct reset_control *usb1_reset;
>>>
>>> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>> +		if (!has_utmi_pad_registers)
>>> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>> +		else
>>> +			usb1_reset = tegra->rst;
>> ...
>>>   		usb1_reset_attempted = true;
>>>   	}
>>
>> This is a pre-existing issue, but what happens if the probes for two USB
>> controllers run in parallel; there seems to be missing locking related to
>> testing/setting usb1_reset_attempted, which could cause multiple parallel
>> attempts to get the "utmi-pads" reset object, which would presumably cause
>> essentially the same issue this patch is solving in other cases?
>
> Hah! Interestingly my initial attempt at fixing this was to introduce a
> lock to serialize these, because I thought that was what was going on. I
> don't think this function can ever run concurrently for different
> devices because the driver core already serializes probes (unless a
> driver specifically requests asynchronous probing, which this one
> doesn't).

Why not just use the new shared reset functionality ? It is easy to use,
that way you can drop some of the special handling in the driver and
you're code actually reflects the hardware (which IMHO has a shared reset).

Regards,

Hans

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Stephen Warren]

On 05/05/2016 02:05 AM, Hans de Goede wrote:
> Hi,
>
> On 04-05-16 22:25, Thierry Reding wrote:
>> On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
>>> On 05/04/2016 08:40 AM, Thierry Reding wrote:
>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
>>>> controls") there is a reference count for reset control assertions. The
>>>> goal is to allow resets to be shared by multiple devices and an assert
>>>> will take effect only when all instances have asserted the reset.
>>>>
>>>> In order to preserve backwards-compatibility, all reset controls become
>>>> exclusive by default. This is to ensure that reset_control_assert() can
>>>> immediately assert in hardware.
>>>>
>>>> However, this new behaviour triggers the following warning in the EHCI
>>>> driver for Tegra:
>>> ...
>>>> The reason is that Tegra SoCs have three EHCI controllers, each with a
>>>> separate reset line. However the first controller contains UTMI pads
>>>> configuration registers that are shared with its siblings and that are
>>>> reset as part of the first controller's reset. There is special code in
>>>> the driver to assert and deassert this shared reset at probe time, and
>>>> it does so irrespective of which controller is probed first to ensure
>>>> that these shared registers are reset before any of the controllers are
>>>> initialized. Unfortunately this means that if the first controller gets
>>>> probed first, it will request its own reset line and will subsequently
>>>> request the same reset line again (temporarily) to perform the reset.
>>>> This used to work fine before the above-mentioned commit, but now
>>>> triggers the new WARN.
>>>>
>>>> Work around this by making sure we reuse the controller's reset if the
>>>> controller happens to be the first controller.
>>>
>>>> diff --git a/drivers/usb/host/ehci-tegra.c
>>>> b/drivers/usb/host/ehci-tegra.c
>>>
>>>> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct
>>>> platform_device *pdev)
>>>
>>>> +    bool has_utmi_pad_registers = false;
>>>>
>>>>       phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>>>>       if (!phy_np)
>>>>           return -ENOENT;
>>>>
>>>> +    if (of_property_read_bool(phy_np,
>>>> "nvidia,has-utmi-pad-registers"))
>>>> +        has_utmi_pad_registers = true;
>>>
>>> Isn't that just:
>>>
>>> has_utmi_pad_registers = of_property_read_bool(phy_np,
>>>     "nvidia,has-utmi-pad-registers");
>>>
>>> ... and then you can remove " = false" from the declaration too?
>>
>> Yes. This is really only for aesthetics. The direct assignment doesn't
>> fit within 80 columns, and wrapping it looks ugly whichever way you do
>> it.
>>
>>>>       if (!usb1_reset_attempted) {
>>>>           struct reset_control *usb1_reset;
>>>>
>>>> -        usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>> +        if (!has_utmi_pad_registers)
>>>> +            usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>> +        else
>>>> +            usb1_reset = tegra->rst;
>>> ...
>>>>           usb1_reset_attempted = true;
>>>>       }
>>>
>>> This is a pre-existing issue, but what happens if the probes for two USB
>>> controllers run in parallel; there seems to be missing locking
>>> related to
>>> testing/setting usb1_reset_attempted, which could cause multiple
>>> parallel
>>> attempts to get the "utmi-pads" reset object, which would presumably
>>> cause
>>> essentially the same issue this patch is solving in other cases?
>>
>> Hah! Interestingly my initial attempt at fixing this was to introduce a
>> lock to serialize these, because I thought that was what was going on. I
>> don't think this function can ever run concurrently for different
>> devices because the driver core already serializes probes (unless a
>> driver specifically requests asynchronous probing, which this one
>> doesn't).
>
> Why not just use the new shared reset functionality ? It is easy to use,
> that way you can drop some of the special handling in the driver and
> you're code actually reflects the hardware (which IMHO has a shared reset).

Judging purely by the descriptions of the shared reset functionality in 
this thread, I doubt that will work. A varying number of USB controllers 
will be enabled in DT on a board-by-board basis, so anything that 
attempts to wait for "all devices to assert reset" can't be implemented, 
since it won't be known ahead of time how many reset assertions to wait 
for. Equally, if device probes are serialized, the reset will not happen 
at the right time since it can't happen until the nth probe (when each 
device has asserted reset) but we want it to happen during the 1st probe.

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Stephen Warren]

On 05/05/2016 02:05 AM, Hans de Goede wrote:
> Hi,
>
> On 04-05-16 22:25, Thierry Reding wrote:
>> On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
>>> On 05/04/2016 08:40 AM, Thierry Reding wrote:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
>>>> controls") there is a reference count for reset control assertions. The
>>>> goal is to allow resets to be shared by multiple devices and an assert
>>>> will take effect only when all instances have asserted the reset.
>>>>
>>>> In order to preserve backwards-compatibility, all reset controls become
>>>> exclusive by default. This is to ensure that reset_control_assert() can
>>>> immediately assert in hardware.
>>>>
>>>> However, this new behaviour triggers the following warning in the EHCI
>>>> driver for Tegra:
>>> ...
>>>> The reason is that Tegra SoCs have three EHCI controllers, each with a
>>>> separate reset line. However the first controller contains UTMI pads
>>>> configuration registers that are shared with its siblings and that are
>>>> reset as part of the first controller's reset. There is special code in
>>>> the driver to assert and deassert this shared reset at probe time, and
>>>> it does so irrespective of which controller is probed first to ensure
>>>> that these shared registers are reset before any of the controllers are
>>>> initialized. Unfortunately this means that if the first controller gets
>>>> probed first, it will request its own reset line and will subsequently
>>>> request the same reset line again (temporarily) to perform the reset.
>>>> This used to work fine before the above-mentioned commit, but now
>>>> triggers the new WARN.
>>>>
>>>> Work around this by making sure we reuse the controller's reset if the
>>>> controller happens to be the first controller.
>>>
>>>> diff --git a/drivers/usb/host/ehci-tegra.c
>>>> b/drivers/usb/host/ehci-tegra.c
>>>
>>>> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct
>>>> platform_device *pdev)
>>>
>>>> +    bool has_utmi_pad_registers = false;
>>>>
>>>>       phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>>>>       if (!phy_np)
>>>>           return -ENOENT;
>>>>
>>>> +    if (of_property_read_bool(phy_np,
>>>> "nvidia,has-utmi-pad-registers"))
>>>> +        has_utmi_pad_registers = true;
>>>
>>> Isn't that just:
>>>
>>> has_utmi_pad_registers = of_property_read_bool(phy_np,
>>>     "nvidia,has-utmi-pad-registers");
>>>
>>> ... and then you can remove " = false" from the declaration too?
>>
>> Yes. This is really only for aesthetics. The direct assignment doesn't
>> fit within 80 columns, and wrapping it looks ugly whichever way you do
>> it.
>>
>>>>       if (!usb1_reset_attempted) {
>>>>           struct reset_control *usb1_reset;
>>>>
>>>> -        usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>> +        if (!has_utmi_pad_registers)
>>>> +            usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>> +        else
>>>> +            usb1_reset = tegra->rst;
>>> ...
>>>>           usb1_reset_attempted = true;
>>>>       }
>>>
>>> This is a pre-existing issue, but what happens if the probes for two USB
>>> controllers run in parallel; there seems to be missing locking
>>> related to
>>> testing/setting usb1_reset_attempted, which could cause multiple
>>> parallel
>>> attempts to get the "utmi-pads" reset object, which would presumably
>>> cause
>>> essentially the same issue this patch is solving in other cases?
>>
>> Hah! Interestingly my initial attempt at fixing this was to introduce a
>> lock to serialize these, because I thought that was what was going on. I
>> don't think this function can ever run concurrently for different
>> devices because the driver core already serializes probes (unless a
>> driver specifically requests asynchronous probing, which this one
>> doesn't).
>
> Why not just use the new shared reset functionality ? It is easy to use,
> that way you can drop some of the special handling in the driver and
> you're code actually reflects the hardware (which IMHO has a shared reset).

Judging purely by the descriptions of the shared reset functionality in 
this thread, I doubt that will work. A varying number of USB controllers 
will be enabled in DT on a board-by-board basis, so anything that 
attempts to wait for "all devices to assert reset" can't be implemented, 
since it won't be known ahead of time how many reset assertions to wait 
for. Equally, if device probes are serialized, the reset will not happen 
at the right time since it can't happen until the nth probe (when each 
device has asserted reset) but we want it to happen during the 1st probe.

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

[Tuomas Tynkkynen]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
...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

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

[Tuomas Tynkkynen]

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

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Jon Hunter]

On 04/05/16 15:40, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
> 
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
> 
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
> 
> [    3.365019] ------------[ cut here ]------------
> [    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
> [    3.382151] Modules linked in:
> [    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
> [    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
> [    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
> [    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
> [    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
> [    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
> [    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
> [    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
> [    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
> [    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
> [    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
> [    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
> [    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
> [    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
> [    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
> [    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
> [    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
> [    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> 
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
> 
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.
> 
> Cc: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v3:
> - reword commit message to more accurately describe the hardware design
> 
> Changes in v2:
> - restore has_utmi_pad_registers condition (Alan Stern)
> 
>  drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..8396b622f238 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>  	struct tegra_ehci_hcd *tegra =
>  		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> +	bool has_utmi_pad_registers = false;
>  
>  	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>  	if (!phy_np)
>  		return -ENOENT;
>  
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;
> +
>  	if (!usb1_reset_attempted) {
>  		struct reset_control *usb1_reset;
>  
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
> +
>  		if (IS_ERR(usb1_reset)) {
>  			dev_warn(&pdev->dev,
>  				 "can't get utmi-pads reset from the PHY\n");
> @@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  			reset_control_deassert(usb1_reset);
>  		}
>  
> -		reset_control_put(usb1_reset);
> +		if (!has_utmi_pad_registers)
> +			reset_control_put(usb1_reset);
> +
>  		usb1_reset_attempted = true;
>  	}
>  
> -	if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
> +	if (!has_utmi_pad_registers) {
>  		reset_control_assert(tegra->rst);
>  		udelay(1);
>  		reset_control_deassert(tegra->rst);
> 

I spent a bit of time looking at this to figure out what it is doing. Can we simply 
this a bit as follows (limited testing so far) ...

Cheers
Jon

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index c1c1024a054c..70501053e1ec 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -81,15 +81,25 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
        struct usb_hcd *hcd = platform_get_drvdata(pdev);
        struct tegra_ehci_hcd *tegra =
                (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
+       bool has_utmi_pad_registers = false;
+       dev_info(&pdev->dev, "%s-%d\n", __func__, __LINE__);
 
        phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
        if (!phy_np)
                return -ENOENT;
 
-       if (!usb1_reset_attempted) {
-               struct reset_control *usb1_reset;
+       if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
+               has_utmi_pad_registers = true;
+
+       /* If we are usb1 and usb1 has been reset, then we are done */
+       if (usb1_reset_attempted && has_utmi_pad_registers)
+               goto out;
 
+       /* If we are not usb1 and usb1 has not been reset, then reset it now */
+       if (!usb1_reset_attempted && !has_utmi_pad_registers) {
+               struct reset_control *usb1_reset;
                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");
@@ -99,18 +109,17 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
                        reset_control_assert(usb1_reset);
                        udelay(1);
                        reset_control_deassert(usb1_reset);
+                       reset_control_put(usb1_reset);
                }

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Jon Hunter]

On 04/05/16 15:40, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
> 
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
> 
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
> 
> [    3.365019] ------------[ cut here ]------------
> [    3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c
> [    3.382151] Modules linked in:
> [    3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140
> [    3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14)
> [    3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4)
> [    3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100)
> [    3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28)
> [    3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c)
> [    3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518)
> [    3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0)
> [    3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330)
> [    3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc)
> [    3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c)
> [    3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218)
> [    3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8)
> [    3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170)
> [    3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8)
> [    3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114)
> [    3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c)
> [    3.529361] ---[ end trace 4bda87dbe4ecef8a ]---
> 
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
> 
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v3:
> - reword commit message to more accurately describe the hardware design
> 
> Changes in v2:
> - restore has_utmi_pad_registers condition (Alan Stern)
> 
>  drivers/usb/host/ehci-tegra.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..8396b622f238 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>  	struct tegra_ehci_hcd *tegra =
>  		(struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> +	bool has_utmi_pad_registers = false;
>  
>  	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>  	if (!phy_np)
>  		return -ENOENT;
>  
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;
> +
>  	if (!usb1_reset_attempted) {
>  		struct reset_control *usb1_reset;
>  
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
> +
>  		if (IS_ERR(usb1_reset)) {
>  			dev_warn(&pdev->dev,
>  				 "can't get utmi-pads reset from the PHY\n");
> @@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>  			reset_control_deassert(usb1_reset);
>  		}
>  
> -		reset_control_put(usb1_reset);
> +		if (!has_utmi_pad_registers)
> +			reset_control_put(usb1_reset);
> +
>  		usb1_reset_attempted = true;
>  	}
>  
> -	if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
> +	if (!has_utmi_pad_registers) {
>  		reset_control_assert(tegra->rst);
>  		udelay(1);
>  		reset_control_deassert(tegra->rst);
> 

I spent a bit of time looking at this to figure out what it is doing. Can we simply 
this a bit as follows (limited testing so far) ...

Cheers
Jon

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index c1c1024a054c..70501053e1ec 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -81,15 +81,25 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
        struct usb_hcd *hcd = platform_get_drvdata(pdev);
        struct tegra_ehci_hcd *tegra =
                (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
+       bool has_utmi_pad_registers = false;
+       dev_info(&pdev->dev, "%s-%d\n", __func__, __LINE__);
 
        phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
        if (!phy_np)
                return -ENOENT;
 
-       if (!usb1_reset_attempted) {
-               struct reset_control *usb1_reset;
+       if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
+               has_utmi_pad_registers = true;
+
+       /* If we are usb1 and usb1 has been reset, then we are done */
+       if (usb1_reset_attempted && has_utmi_pad_registers)
+               goto out;
 
+       /* If we are not usb1 and usb1 has not been reset, then reset it now */
+       if (!usb1_reset_attempted && !has_utmi_pad_registers) {
+               struct reset_control *usb1_reset;
                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");
@@ -99,18 +109,17 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
                        reset_control_assert(usb1_reset);
                        udelay(1);
                        reset_control_deassert(usb1_reset);
+                       reset_control_put(usb1_reset);
                }
-
-               reset_control_put(usb1_reset);
-               usb1_reset_attempted = true;
        }
 
-       if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) {
-               reset_control_assert(tegra->rst);
-               udelay(1);
-               reset_control_deassert(tegra->rst);
-       }
+       reset_control_assert(tegra->rst);
+       udelay(1);
+       reset_control_deassert(tegra->rst);
+
+       usb1_reset_attempted = true;
 
+out:
        of_node_put(phy_np);
 
        return 0;

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Jon Hunter]

On 05/05/16 18:05, Jon Hunter wrote:

[snip]

> I spent a bit of time looking at this to figure out what it is doing. Can we simply 
> this a bit as follows (limited testing so far) ...
> 
> Cheers
> Jon
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..70501053e1ec 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,25 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>         struct usb_hcd *hcd = platform_get_drvdata(pdev);
>         struct tegra_ehci_hcd *tegra =
>                 (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> +       bool has_utmi_pad_registers = false;
> +       dev_info(&pdev->dev, "%s-%d\n", __func__, __LINE__);

Oops, some left over debug I meant to remove :-p
Jon

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Jon Hunter]

On 05/05/16 18:05, Jon Hunter wrote:

[snip]

> I spent a bit of time looking at this to figure out what it is doing. Can we simply 
> this a bit as follows (limited testing so far) ...
> 
> Cheers
> Jon
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index c1c1024a054c..70501053e1ec 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -81,15 +81,25 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)
>         struct usb_hcd *hcd = platform_get_drvdata(pdev);
>         struct tegra_ehci_hcd *tegra =
>                 (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv;
> +       bool has_utmi_pad_registers = false;
> +       dev_info(&pdev->dev, "%s-%d\n", __func__, __LINE__);

Oops, some left over debug I meant to remove :-p
Jon

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Hans de Goede]

Hi,

On 05-05-16 18:00, Stephen Warren wrote:
> On 05/05/2016 02:05 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 04-05-16 22:25, Thierry Reding wrote:
>>> On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
>>>> On 05/04/2016 08:40 AM, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
>>>>> controls") there is a reference count for reset control assertions. The
>>>>> goal is to allow resets to be shared by multiple devices and an assert
>>>>> will take effect only when all instances have asserted the reset.
>>>>>
>>>>> In order to preserve backwards-compatibility, all reset controls become
>>>>> exclusive by default. This is to ensure that reset_control_assert() can
>>>>> immediately assert in hardware.
>>>>>
>>>>> However, this new behaviour triggers the following warning in the EHCI
>>>>> driver for Tegra:
>>>> ...
>>>>> The reason is that Tegra SoCs have three EHCI controllers, each with a
>>>>> separate reset line. However the first controller contains UTMI pads
>>>>> configuration registers that are shared with its siblings and that are
>>>>> reset as part of the first controller's reset. There is special code in
>>>>> the driver to assert and deassert this shared reset at probe time, and
>>>>> it does so irrespective of which controller is probed first to ensure
>>>>> that these shared registers are reset before any of the controllers are
>>>>> initialized. Unfortunately this means that if the first controller gets
>>>>> probed first, it will request its own reset line and will subsequently
>>>>> request the same reset line again (temporarily) to perform the reset.
>>>>> This used to work fine before the above-mentioned commit, but now
>>>>> triggers the new WARN.
>>>>>
>>>>> Work around this by making sure we reuse the controller's reset if the
>>>>> controller happens to be the first controller.
>>>>
>>>>> diff --git a/drivers/usb/host/ehci-tegra.c
>>>>> b/drivers/usb/host/ehci-tegra.c
>>>>
>>>>> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct
>>>>> platform_device *pdev)
>>>>
>>>>> +    bool has_utmi_pad_registers = false;
>>>>>
>>>>>       phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>>>>>       if (!phy_np)
>>>>>           return -ENOENT;
>>>>>
>>>>> +    if (of_property_read_bool(phy_np,
>>>>> "nvidia,has-utmi-pad-registers"))
>>>>> +        has_utmi_pad_registers = true;
>>>>
>>>> Isn't that just:
>>>>
>>>> has_utmi_pad_registers = of_property_read_bool(phy_np,
>>>>     "nvidia,has-utmi-pad-registers");
>>>>
>>>> ... and then you can remove " = false" from the declaration too?
>>>
>>> Yes. This is really only for aesthetics. The direct assignment doesn't
>>> fit within 80 columns, and wrapping it looks ugly whichever way you do
>>> it.
>>>
>>>>>       if (!usb1_reset_attempted) {
>>>>>           struct reset_control *usb1_reset;
>>>>>
>>>>> -        usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>>> +        if (!has_utmi_pad_registers)
>>>>> +            usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>>> +        else
>>>>> +            usb1_reset = tegra->rst;
>>>> ...
>>>>>           usb1_reset_attempted = true;
>>>>>       }
>>>>
>>>> This is a pre-existing issue, but what happens if the probes for two USB
>>>> controllers run in parallel; there seems to be missing locking
>>>> related to
>>>> testing/setting usb1_reset_attempted, which could cause multiple
>>>> parallel
>>>> attempts to get the "utmi-pads" reset object, which would presumably
>>>> cause
>>>> essentially the same issue this patch is solving in other cases?
>>>
>>> Hah! Interestingly my initial attempt at fixing this was to introduce a
>>> lock to serialize these, because I thought that was what was going on. I
>>> don't think this function can ever run concurrently for different
>>> devices because the driver core already serializes probes (unless a
>>> driver specifically requests asynchronous probing, which this one
>>> doesn't).
>>
>> Why not just use the new shared reset functionality ? It is easy to use,
>> that way you can drop some of the special handling in the driver and
>> you're code actually reflects the hardware (which IMHO has a shared reset).
>
> Judging purely by the descriptions of the shared reset functionality in this thread, I doubt that will work. A varying number of USB controllers will be enabled in DT on a board-by-board basis, so anything that attempts to wait for "all devices to assert reset" can't be implemented, since it won't be known ahead of time how many reset assertions to wait for. Equally, if device probes are serialized, the reset will not happen at the right time since it can't happen until the nth probe (when each device has asserted reset) but we want it to happen during the 1st probe.

Ah, so you actually want to reset the utmi-pad registers, not take them
out of reset state ?

Yeah then the shared reset support will not work.

Regards,

Hans

>

Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

[Hans de Goede]

Hi,

On 05-05-16 18:00, Stephen Warren wrote:
> On 05/05/2016 02:05 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 04-05-16 22:25, Thierry Reding wrote:
>>> On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote:
>>>> On 05/04/2016 08:40 AM, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
>>>>> controls") there is a reference count for reset control assertions. The
>>>>> goal is to allow resets to be shared by multiple devices and an assert
>>>>> will take effect only when all instances have asserted the reset.
>>>>>
>>>>> In order to preserve backwards-compatibility, all reset controls become
>>>>> exclusive by default. This is to ensure that reset_control_assert() can
>>>>> immediately assert in hardware.
>>>>>
>>>>> However, this new behaviour triggers the following warning in the EHCI
>>>>> driver for Tegra:
>>>> ...
>>>>> The reason is that Tegra SoCs have three EHCI controllers, each with a
>>>>> separate reset line. However the first controller contains UTMI pads
>>>>> configuration registers that are shared with its siblings and that are
>>>>> reset as part of the first controller's reset. There is special code in
>>>>> the driver to assert and deassert this shared reset at probe time, and
>>>>> it does so irrespective of which controller is probed first to ensure
>>>>> that these shared registers are reset before any of the controllers are
>>>>> initialized. Unfortunately this means that if the first controller gets
>>>>> probed first, it will request its own reset line and will subsequently
>>>>> request the same reset line again (temporarily) to perform the reset.
>>>>> This used to work fine before the above-mentioned commit, but now
>>>>> triggers the new WARN.
>>>>>
>>>>> Work around this by making sure we reuse the controller's reset if the
>>>>> controller happens to be the first controller.
>>>>
>>>>> diff --git a/drivers/usb/host/ehci-tegra.c
>>>>> b/drivers/usb/host/ehci-tegra.c
>>>>
>>>>> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct
>>>>> platform_device *pdev)
>>>>
>>>>> +    bool has_utmi_pad_registers = false;
>>>>>
>>>>>       phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>>>>>       if (!phy_np)
>>>>>           return -ENOENT;
>>>>>
>>>>> +    if (of_property_read_bool(phy_np,
>>>>> "nvidia,has-utmi-pad-registers"))
>>>>> +        has_utmi_pad_registers = true;
>>>>
>>>> Isn't that just:
>>>>
>>>> has_utmi_pad_registers = of_property_read_bool(phy_np,
>>>>     "nvidia,has-utmi-pad-registers");
>>>>
>>>> ... and then you can remove " = false" from the declaration too?
>>>
>>> Yes. This is really only for aesthetics. The direct assignment doesn't
>>> fit within 80 columns, and wrapping it looks ugly whichever way you do
>>> it.
>>>
>>>>>       if (!usb1_reset_attempted) {
>>>>>           struct reset_control *usb1_reset;
>>>>>
>>>>> -        usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>>> +        if (!has_utmi_pad_registers)
>>>>> +            usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
>>>>> +        else
>>>>> +            usb1_reset = tegra->rst;
>>>> ...
>>>>>           usb1_reset_attempted = true;
>>>>>       }
>>>>
>>>> This is a pre-existing issue, but what happens if the probes for two USB
>>>> controllers run in parallel; there seems to be missing locking
>>>> related to
>>>> testing/setting usb1_reset_attempted, which could cause multiple
>>>> parallel
>>>> attempts to get the "utmi-pads" reset object, which would presumably
>>>> cause
>>>> essentially the same issue this patch is solving in other cases?
>>>
>>> Hah! Interestingly my initial attempt at fixing this was to introduce a
>>> lock to serialize these, because I thought that was what was going on. I
>>> don't think this function can ever run concurrently for different
>>> devices because the driver core already serializes probes (unless a
>>> driver specifically requests asynchronous probing, which this one
>>> doesn't).
>>
>> Why not just use the new shared reset functionality ? It is easy to use,
>> that way you can drop some of the special handling in the driver and
>> you're code actually reflects the hardware (which IMHO has a shared reset).
>
> Judging purely by the descriptions of the shared reset functionality in this thread, I doubt that will work. A varying number of USB controllers will be enabled in DT on a board-by-board basis, so anything that attempts to wait for "all devices to assert reset" can't be implemented, since it won't be known ahead of time how many reset assertions to wait for. Equally, if device probes are serialized, the reset will not happen at the right time since it can't happen until the nth probe (when each device has asserted reset) but we want it to happen during the 1st probe.

Ah, so you actually want to reset the utmi-pad registers, not take them
out of reset state ?

Yeah then the shared reset support will not work.

Regards,

Hans

>