Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
From: Hans de Goede
Date: Thu May 05 2016 - 04:05:56 EST
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@xxxxxxxxxx>
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