[PATCH 0/3] Tegra USB probe order issue fix

[PATCH 0/3] Tegra USB probe order issue fix

[Tuomas Tynkkynen]

Hi all,

This series fixes a probe order issue with the Tegra EHCI driver.
Basically, the register area of the 1st USB controller contains some
registers that are global to all of the controllers, but that are also
cleared when reset is asserted to the 1st controller. So if (say) the
3rd controller would be the first one to finish probing successfully,
then the reset that happens during the 1st controller's probe would
result in broken USB. So the before doing anything with the USB HW,
we should reset the 1st controller once, and then never ever reset
it again.

However, fixing this without breaking existing device trees is very
complicated, as the 2nd and 3rd Tegra EHCI controllers don't have a
phandle to the 1st controller's reset handle, so we have to find it
the hard way. This is done in patch 2.

Patch 1 is a prerequisite change to the reset control API, so that we
can get a reset controller reference without a struct device *.

While testing that the 1st USB controller still works without a reset
when the driver is unbound and bound again, I discovered an unbalanced
regulator_disable + clk_disable_unprepare in the PHY code if the EHCI
driver is unbound and rebound. This is fixed in patch 3.

Tested on the Jetson TK1 that the 3rd USB port still works when the
1st controller is enabled and moved so that it's the last USB
node in the DT. Also tested that the 1st controller still works
after an unbind + rebind, even though it's not getting resetted
during the probe.

Thanks,
Tuomas

Tuomas Tynkkynen (3):
  reset: Re-export of_reset_control_get
  USB: EHCI: tegra: Fix probe order issue leading to broken USB
  USB: PHY: tegra: Call tegra_usb_phy_close only on device removal

 drivers/usb/host/ehci-tegra.c   | 111 ++++++++++++++++++++++++++++++++++++++--
 drivers/usb/phy/phy-tegra-usb.c |   8 ++-
 include/linux/reset.h           |   8 +++
 3 files changed, 119 insertions(+), 8 deletions(-)

-- 
1.8.1.5

[PATCH 1/3] reset: Re-export of_reset_control_get

[Tuomas Tynkkynen]

Commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f (reset: Add optional
resets and stubs) accidentally dropped the declaration of
of_reset_control_get from include/linux/reset.h. Add it back to the
header.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 include/linux/reset.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/reset.h b/include/linux/reset.h
index 349f150..cfc8de8 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -13,6 +13,8 @@ int reset_control_deassert(struct reset_control *rstc);
 
 struct reset_control *reset_control_get(struct device *dev, const char *id);
 void reset_control_put(struct reset_control *rstc);
+struct reset_control *of_reset_control_get(struct device_node *node,
+					   const char *id);
 struct reset_control *devm_reset_control_get(struct device *dev, const char *id);
 
 int __must_check device_reset(struct device *dev);
@@ -73,6 +75,12 @@ static inline struct reset_control *reset_control_get_optional(
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct reset_control *of_reset_control_get(struct device_node *np,
+							 const char *id)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline struct reset_control *devm_reset_control_get_optional(
 					struct device *dev, const char *id)
 {
-- 
1.8.1.5

[PATCH 2/3] USB: EHCI: tegra: Fix probe order issue leading to broken USB

[Tuomas Tynkkynen]

The Tegra USB complex has a particularly annoying misdesign: some of the
UTMI pad configuration registers are global for all the 3 USB controllers
on the chip, but those registers are located in the first controller's
register space and will be cleared when the reset to the first
controller is asserted. Currently, this means that if the 1st controller
were to finish probing after the 2nd or 3rd controller, USB would not
work at all.

Fix this situation by always resetting the 1st controller before doing
any other setup to any of the controllers, and then never ever reset the
first controller again.

The really ugly part is that the EHCI controllers don't have a reset
control phandle to the 1st controller, so we have to do some device tree
groveling to find the phandle to the 1st controller.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/usb/host/ehci-tegra.c | 111 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 108 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 5590567..e3f362b 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -24,6 +24,7 @@
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/platform_device.h>
@@ -46,6 +47,7 @@
 #define DRV_NAME "tegra-ehci"
 
 static struct hc_driver __read_mostly tegra_ehci_hc_driver;
+static struct device_node *usb1_ehci_node;
 
 struct tegra_ehci_soc_config {
 	bool has_hostpc;
@@ -60,6 +62,108 @@ struct tegra_ehci_hcd {
 	enum tegra_usb_phy_port_speed port_speed;
 };
 
+/*
+ * The 1st USB controller contains some PHY registers that are global for
+ * all the controllers on the chip. Those registers are also cleared when
+ * reset is asserted to the 1st controller. This means that the 1st controller
+ * can only be reset when no other controlled has finished probing. So we'll
+ * reset the 1st controller before doing any other setup on any of the
+ * controllers, and then never again.
+ *
+ * Sadly, the EHCI device tree nodes don't have the phandle to the first USB
+ * controller, so in order not to break the DT ABI, this hack is required to
+ * locate it.
+ */
+static struct device_node *tegra_find_usb1_node(struct platform_device *pdev)
+{
+	int err;
+	int num = 0;
+	const char *compatible;
+	struct device_node *np, *lowest_node = NULL;
+	u64 lowest_addr = U64_MAX;
+
+	/*
+	 * Find the node that has the same compatible string as &pdev->dev
+	 * and has the lowest register address.
+	 */
+	of_property_read_string(pdev->dev.of_node, "compatible",
+				&compatible);
+
+	for_each_compatible_node(np, NULL, compatible) {
+		u64 sz, address;
+		const __be32 *addr_cells;
+
+		addr_cells = of_get_address(np, 0, &sz, NULL);
+		if (!addr_cells) {
+			dev_err(&pdev->dev, "found EHCI node with no address");
+			of_node_put(np);
+			goto failed;
+		}
+
+		address = of_read_number(addr_cells, of_n_addr_cells(np));
+		if (address < lowest_addr) {
+			of_node_put(lowest_node);
+			lowest_node = np;
+			lowest_addr = address;
+		} else {
+			of_node_put(np);
+		}
+		num++;
+	}
+
+	if (num != 3) {
+		dev_err(&pdev->dev, "couldn't locate all 3 EHCI nodes");
+		err = -ENODEV;
+		goto failed;
+	}
+
+	return lowest_node;
+
+failed:
+	of_node_put(lowest_node);
+
+	return ERR_PTR(err);
+}
+
+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;
+
+	if (!usb1_ehci_node) {
+		struct device_node *np;
+		struct reset_control *usb1_reset;
+
+		np = tegra_find_usb1_node(pdev);
+		if (!np)
+			return -ENODEV;
+
+		usb1_reset = of_reset_control_get(np, "usb");
+		if (IS_ERR(usb1_reset)) {
+			dev_err(&pdev->dev,
+				"couldn't get reset for 1st USB controller");
+			of_node_put(np);
+			return PTR_ERR(usb1_reset);
+		}
+
+		reset_control_assert(usb1_reset);
+		udelay(1);
+		reset_control_deassert(usb1_reset);
+
+		reset_control_put(usb1_reset);
+		usb1_ehci_node = np;
+	}
+
+	if (pdev->dev.of_node != usb1_ehci_node) {
+		reset_control_assert(tegra->rst);
+		udelay(1);
+		reset_control_deassert(tegra->rst);
+	}
+
+	return 0;
+}
+
 static int tegra_ehci_internal_port_reset(
 	struct ehci_hcd	*ehci,
 	u32 __iomem	*portsc_reg
@@ -389,9 +493,9 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	if (err)
 		goto cleanup_hcd_create;
 
-	reset_control_assert(tegra->rst);
-	udelay(1);
-	reset_control_deassert(tegra->rst);
+	err = tegra_reset_usb_controller(pdev);
+	if (err)
+		goto cleanup_clk_en;
 
 	u_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
 	if (IS_ERR(u_phy)) {
@@ -560,6 +664,7 @@ module_init(ehci_tegra_init);
 
 static void __exit ehci_tegra_cleanup(void)
 {
+	of_node_put(usb1_ehci_node);
 	platform_driver_unregister(&tegra_ehci_driver);
 }
 module_exit(ehci_tegra_cleanup);
-- 
1.8.1.5

[PATCH 3/3] USB: PHY: tegra: Call tegra_usb_phy_close only on device removal

[Tuomas Tynkkynen]

tegra_usb_phy_close() is supposed to undo the effects of
tegra_usb_phy_init(). It is also currently added as the USB PHY shutdown
callback, which is wrong, since tegra_usb_phy_init() is only called
during probing wheras the shutdown callback can get called multiple
times. This then leads to warnings about unbalanced regulator_disable if
the EHCI driver is unbound and bound again at runtime.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index bbe4f8e..72e04a9 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -686,10 +686,8 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
 	return gpio_direction_output(phy->reset_gpio, 0);
 }
 
-static void tegra_usb_phy_close(struct usb_phy *x)
+static void tegra_usb_phy_close(struct tegra_usb_phy *phy)
 {
-	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
-
 	if (!IS_ERR(phy->vbus))
 		regulator_disable(phy->vbus);
 
@@ -1061,14 +1059,13 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
-	tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
 	tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
 
 	platform_set_drvdata(pdev, tegra_phy);
 
 	err = usb_add_phy_dev(&tegra_phy->u_phy);
 	if (err < 0) {
-		tegra_usb_phy_close(&tegra_phy->u_phy);
+		tegra_usb_phy_close(tegra_phy);
 		return err;
 	}
 
@@ -1080,6 +1077,7 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
 	struct tegra_usb_phy *tegra_phy = platform_get_drvdata(pdev);
 
 	usb_remove_phy(&tegra_phy->u_phy);
+	tegra_usb_phy_close(tegra_phy);
 
 	return 0;
 }
-- 
1.8.1.5

Re: [PATCH 3/3] USB: PHY: tegra: Call tegra_usb_phy_close only on device removal

[Stephen Warren]

On 07/01/2014 03:08 PM, Tuomas Tynkkynen wrote:
> tegra_usb_phy_close() is supposed to undo the effects of
> tegra_usb_phy_init(). It is also currently added as the USB PHY shutdown
> callback, which is wrong, since tegra_usb_phy_init() is only called
> during probing wheras the shutdown callback can get called multiple
> times. This then leads to warnings about unbalanced regulator_disable if
> the EHCI driver is unbound and bound again at runtime.

The series,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

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

> -static void tegra_usb_phy_close(struct usb_phy *x)
> +static void tegra_usb_phy_close(struct tegra_usb_phy *phy)

If this function undoes what _init does, it seems it should be called
_fini not _close. But that's bike-shedding perhaps.

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Alan Stern]

On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:

> Hi all,
> 
> This series fixes a probe order issue with the Tegra EHCI driver.
> Basically, the register area of the 1st USB controller contains some
> registers that are global to all of the controllers, but that are also
> cleared when reset is asserted to the 1st controller. So if (say) the
> 3rd controller would be the first one to finish probing successfully,
> then the reset that happens during the 1st controller's probe would
> result in broken USB. So the before doing anything with the USB HW,
> we should reset the 1st controller once, and then never ever reset
> it again.

This sounds very much like the sort of thing that ought to be described 
in DT.  It is a hardware dependence, and DT exists for the purpose of 
describing the hardware.

Alan Stern

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Stephen Warren]

On 07/02/2014 08:02 AM, Alan Stern wrote:
> On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
> 
>> Hi all,
>>
>> This series fixes a probe order issue with the Tegra EHCI driver.
>> Basically, the register area of the 1st USB controller contains some
>> registers that are global to all of the controllers, but that are also
>> cleared when reset is asserted to the 1st controller. So if (say) the
>> 3rd controller would be the first one to finish probing successfully,
>> then the reset that happens during the 1st controller's probe would
>> result in broken USB. So the before doing anything with the USB HW,
>> we should reset the 1st controller once, and then never ever reset
>> it again.
> 
> This sounds very much like the sort of thing that ought to be described 
> in DT.  It is a hardware dependence, and DT exists for the purpose of 
> describing the hardware.

DT is more about describing the HW, not how SW has to use the HW.
probe() ordering is a SW issue, not a HW description. It's driver
knowledge that the HW resets have to run in a certain order, and if the
driver didn't actually reset the HW ever (but rather, re-programmed all
registers so reset was never needed), then order wouldn't be relevant

DT certainly doesn't have any mechanism for describing probe order or
anything like that, although you can fake it out by adding phandles
between nodes, and having SW wait for the driver for the referenced node
to probe first. That won't work here though, since there's no guarantee
that the USB1 node will actually be enabled (that USB port might not be
hooked up on the board, hence the DT node will be disabled), so we can't
rely on a driver for it ever appearing.

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Alan Stern]

On Wed, 2 Jul 2014, Stephen Warren wrote:

> On 07/02/2014 08:02 AM, Alan Stern wrote:
> > On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
> > 
> >> Hi all,
> >>
> >> This series fixes a probe order issue with the Tegra EHCI driver.
> >> Basically, the register area of the 1st USB controller contains some
> >> registers that are global to all of the controllers, but that are also
> >> cleared when reset is asserted to the 1st controller. So if (say) the
> >> 3rd controller would be the first one to finish probing successfully,
> >> then the reset that happens during the 1st controller's probe would
> >> result in broken USB. So the before doing anything with the USB HW,
> >> we should reset the 1st controller once, and then never ever reset
> >> it again.
> > 
> > This sounds very much like the sort of thing that ought to be described 
> > in DT.  It is a hardware dependence, and DT exists for the purpose of 
> > describing the hardware.
> 
> DT is more about describing the HW, not how SW has to use the HW.

Tuomas wrote: "the register area of the 1st USB controller contains
some registers that are global to all of the controllers, but that are
also cleared when reset is asserted to the 1st controller."  That is
very much an attribute of the hardware and so DT should describe it.

> probe() ordering is a SW issue, not a HW description. It's driver
> knowledge that the HW resets have to run in a certain order, and if the
> driver didn't actually reset the HW ever (but rather, re-programmed all
> registers so reset was never needed), then order wouldn't be relevant
> 
> DT certainly doesn't have any mechanism for describing probe order or
> anything like that, although you can fake it out by adding phandles
> between nodes, and having SW wait for the driver for the referenced node
> to probe first. That won't work here though, since there's no guarantee
> that the USB1 node will actually be enabled (that USB port might not be
> hooked up on the board, hence the DT node will be disabled), so we can't
> rely on a driver for it ever appearing.

I wasn't talking about probe order; I was talking about the fact that 
registers pertinent to the later controllers are in the reset domain of 
the first controller.

Alan Stern

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Stephen Warren]

On 07/02/2014 10:09 AM, Alan Stern wrote:
> On Wed, 2 Jul 2014, Stephen Warren wrote:
> 
>> On 07/02/2014 08:02 AM, Alan Stern wrote:
>>> On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
>>>
>>>> Hi all,
>>>>
>>>> This series fixes a probe order issue with the Tegra EHCI driver.
>>>> Basically, the register area of the 1st USB controller contains some
>>>> registers that are global to all of the controllers, but that are also
>>>> cleared when reset is asserted to the 1st controller. So if (say) the
>>>> 3rd controller would be the first one to finish probing successfully,
>>>> then the reset that happens during the 1st controller's probe would
>>>> result in broken USB. So the before doing anything with the USB HW,
>>>> we should reset the 1st controller once, and then never ever reset
>>>> it again.
>>>
>>> This sounds very much like the sort of thing that ought to be described 
>>> in DT.  It is a hardware dependence, and DT exists for the purpose of 
>>> describing the hardware.
>>
>> DT is more about describing the HW, not how SW has to use the HW.
> 
> Tuomas wrote: "the register area of the 1st USB controller contains
> some registers that are global to all of the controllers, but that are
> also cleared when reset is asserted to the 1st controller."  That is
> very much an attribute of the hardware and so DT should describe it.

So you want to add a Boolean DT property to the first USB controller
node indicating that it has the "shared" reset? That would be fine, but
would only replace the content of tegra_find_usb1_node(); much of the
rest of the patch would remain the same.

I guess in this case, it's fine to require DT changes to enable the new
feature of fixing this issue, so long as the code continues to work the
same as it currently does with old DTs. Due to the backwards
compatibility requirement, the patch will end up slightly more complex,
but hopefully not too bad.

>> probe() ordering is a SW issue, not a HW description. It's driver
>> knowledge that the HW resets have to run in a certain order, and if the
>> driver didn't actually reset the HW ever (but rather, re-programmed all
>> registers so reset was never needed), then order wouldn't be relevant
>>
>> DT certainly doesn't have any mechanism for describing probe order or
>> anything like that, although you can fake it out by adding phandles
>> between nodes, and having SW wait for the driver for the referenced node
>> to probe first. That won't work here though, since there's no guarantee
>> that the USB1 node will actually be enabled (that USB port might not be
>> hooked up on the board, hence the DT node will be disabled), so we can't
>> rely on a driver for it ever appearing.
> 
> I wasn't talking about probe order; I was talking about the fact that 
> registers pertinent to the later controllers are in the reset domain of 
> the first controller.
> 
> Alan Stern

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Alan Stern]

On Wed, 2 Jul 2014, Stephen Warren wrote:

> On 07/02/2014 10:09 AM, Alan Stern wrote:
> > On Wed, 2 Jul 2014, Stephen Warren wrote:
> > 
> >> On 07/02/2014 08:02 AM, Alan Stern wrote:
> >>> On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> This series fixes a probe order issue with the Tegra EHCI driver.
> >>>> Basically, the register area of the 1st USB controller contains some
> >>>> registers that are global to all of the controllers, but that are also
> >>>> cleared when reset is asserted to the 1st controller. So if (say) the
> >>>> 3rd controller would be the first one to finish probing successfully,
> >>>> then the reset that happens during the 1st controller's probe would
> >>>> result in broken USB. So the before doing anything with the USB HW,
> >>>> we should reset the 1st controller once, and then never ever reset
> >>>> it again.
> >>>
> >>> This sounds very much like the sort of thing that ought to be described 
> >>> in DT.  It is a hardware dependence, and DT exists for the purpose of 
> >>> describing the hardware.
> >>
> >> DT is more about describing the HW, not how SW has to use the HW.
> > 
> > Tuomas wrote: "the register area of the 1st USB controller contains
> > some registers that are global to all of the controllers, but that are
> > also cleared when reset is asserted to the 1st controller."  That is
> > very much an attribute of the hardware and so DT should describe it.
> 
> So you want to add a Boolean DT property to the first USB controller
> node indicating that it has the "shared" reset?

Something like that, yes.

> That would be fine, but
> would only replace the content of tegra_find_usb1_node(); much of the
> rest of the patch would remain the same.

That's okay.  tegra_find_usb1_node() was the most objectionable part of
the patch.

> I guess in this case, it's fine to require DT changes to enable the new
> feature of fixing this issue, so long as the code continues to work the
> same as it currently does with old DTs. Due to the backwards
> compatibility requirement, the patch will end up slightly more complex,
> but hopefully not too bad.

Good.

Alan Stern