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).
Thierry
Attachment:
signature.asc
Description: PGP signature
